Skip to content

write n2o and ch4 flux variables to SIPNET netCDF#3816

Merged
dlebauer merged 7 commits into
PecanProject:developfrom
divine7022:n2o-ch4
Feb 19, 2026
Merged

write n2o and ch4 flux variables to SIPNET netCDF#3816
dlebauer merged 7 commits into
PecanProject:developfrom
divine7022:n2o-ch4

Conversation

@divine7022

Copy link
Copy Markdown
Member

Description

  • updates model2netcdf.SIPNET to include n2o and ch4 flux variables
  • add test

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I agree that PEcAn Project may distribute my contribution under any or all of
    • the same license as the existing code,
    • and/or the BSD 3-clause license.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment thread models/sipnet/R/model2netcdf.SIPNET.R Outdated
outdir <- withr::local_tempdir(pattern = "sipnet_out_")
rundir <- withr::local_tempdir(pattern = "sipnet_run_")

# minimal sipnet.param — model2netcdf only reads leafCSpWt for LAI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but: Given we go to the trouble of reading leafCSpWt from the param file, why does it get combined with a constant leafC <- 0.48 instead of reading cFracLeaf from the very next param line?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! opened PR #3820

Comment on lines +7 to +8
c("plantWoodInit\t30000\t0\t6600\t14000\t200",
"leafCSpWt\t32\t0\t13\t500\t0"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sipnet v2 drops the extra numeric columns and I would hope it's safe to do that here too:

Suggested change
c("plantWoodInit\t30000\t0\t6600\t14000\t200",
"leafCSpWt\t32\t0\t13\t500\t0"),
c("plantWoodInit\t30000",
"leafCSpWt\t32"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: is the plantWoodInit line needed for this test to work?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, plantWoodInit is not needed. model2netcdf.SIPNET only reads leafCSpWt from the param file. I will trim sipnet.param down to just that one line

Comment thread models/sipnet/tests/testthat/test-model2netcdf.SIPNET.R Outdated
Comment thread models/sipnet/tests/testthat/test-model2netcdf.SIPNET.R Outdated
Comment thread models/sipnet/tests/testthat/test-model2netcdf.SIPNET.R Outdated
Comment thread models/sipnet/tests/testthat/test-model2netcdf.SIPNET.R Outdated
Comment on lines +66 to +71
nc_file <- file.path(real_outdir, "2002.nc")
expect_true(file.exists(nc_file))

nc <- ncdf4::nc_open(nc_file)
on.exit(ncdf4::nc_close(nc), add = TRUE)
vars <- names(nc$var)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests like this seem like a great use case for the netcdf2df() I proposed in #3788 (which isn't merged yet, so no foul for not using it here!) -- one line to slurp the test file into a df instead of needing to remember these incantations every time.

Comment thread models/sipnet/tests/testthat/test-model2netcdf.SIPNET.R Outdated
Comment thread models/sipnet/tests/testthat/test-model2netcdf.SIPNET.R Outdated
Comment on lines +67 to +69
n2o <- as.numeric(ncdf4::ncvar_get(nc, "N2O_flux"))
ch4 <- as.numeric(ncdf4::ncvar_get(nc, "CH4_flux"))
gpp <- as.numeric(ncdf4::ncvar_get(nc, "GPP"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these should be stored as numeric already, and if they aren't then we want to know (as the expect_equal below will do if we skip coercion)

Suggested change
n2o <- as.numeric(ncdf4::ncvar_get(nc, "N2O_flux"))
ch4 <- as.numeric(ncdf4::ncvar_get(nc, "CH4_flux"))
gpp <- as.numeric(ncdf4::ncvar_get(nc, "GPP"))
n2o <- ncdf4::ncvar_get(nc, "N2O_flux")
ch4 <- ncdf4::ncvar_get(nc, "CH4_flux")
gpp <- ncdf4::ncvar_get(nc, "GPP")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right! that these should already be numeric , as.numeric() was masking the type check. But ncvar_get() returns a 1-D array (carries dim attribute), so bare comparison with rep() fails on attributes. Switched to as.vector(), which strips the array dimension without coercing type

@dlebauer dlebauer enabled auto-merge February 19, 2026 16:33
@dlebauer dlebauer added this pull request to the merge queue Feb 19, 2026
Merged via the queue into PecanProject:develop with commit c00644b Feb 19, 2026
28 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants