From 58855f543b6193e726bc43fb3d3e01ceae795c7e Mon Sep 17 00:00:00 2001 From: Man Date: Sat, 28 Mar 2026 10:14:57 +0530 Subject: [PATCH 1/4] fix(uncertainty): resolve median (q50) drop in manifest-based SA lookup read.sa.output() performed a strict per-trait/per-PFT lookup, but write.sa.configs() writes the median run as a single shared manifest row with pft_name='NA' and trait='NA'. This mismatch caused the q50 cell to always be NA in sensitivity.output in the manifest-based OAT SA path. Fix: add a fallback in read.sa.output() -- when quantile == '50' and no exact trait/PFT match is found, look up the shared median entry (pft_name='NA', trait='NA') before issuing a warning. The manifest format written by write.sa.configs() is unchanged. Also adds two regression tests: - happy path: shared median row resolves correctly, no NA - negative: warns when neither exact nor fallback row exists Fixes #3882 --- modules/uncertainty/NEWS.md | 7 ++ modules/uncertainty/R/sensitivity.R | 18 ++++ .../testthat/test-read_sa_output_median.R | 99 +++++++++++++++++++ 3 files changed, 124 insertions(+) create mode 100644 modules/uncertainty/tests/testthat/test-read_sa_output_median.R diff --git a/modules/uncertainty/NEWS.md b/modules/uncertainty/NEWS.md index 5a446773800..e3ff40aab87 100644 --- a/modules/uncertainty/NEWS.md +++ b/modules/uncertainty/NEWS.md @@ -5,6 +5,13 @@ sensitivity analysis. This function ensures non-parameter inputs (met, IC, soil) are held constant, which is required for valid variance decomposition in one-at-a-time (OAT) sensitivity analysis (#3729) +* Fixed `read.sa.output()` dropping the median (q50) row when using the + manifest-based OAT sensitivity path. `write.sa.configs()` writes a single + shared median run with `pft_name = "NA"` and `trait = "NA"`, but the lookup + previously required an exact per-trait/per-PFT match, so the cell was always + `NA`. The fix adds a fallback: when `quantile == "50"` and no exact match is + found, the function now resolves the shared median entry before emitting a + warning. diff --git a/modules/uncertainty/R/sensitivity.R b/modules/uncertainty/R/sensitivity.R index 916e4e4ea07..3370fc8b6e3 100644 --- a/modules/uncertainty/R/sensitivity.R +++ b/modules/uncertainty/R/sensitivity.R @@ -50,6 +50,24 @@ read.sa.output <- function(traits, quantiles, pecandir, outdir, pft.name = "", } else if (nrow(subset_df) > 1) { PEcAn.logger::logger.warn("Multiple runs found for", trait, quantile, "- using the last one.") run.id <- utils::tail(subset_df$run_id, 1) + } else if (as.character(quantile) == "50") { + # The median run is written as a single shared entry with pft_name = "NA" + # and trait = "NA". Fall back to that shared row when no exact match exists. + median_df <- manifest[ + manifest$type == "Sensitivity" & + manifest$pft_name == "NA" & + manifest$trait == "NA" & + as.character(manifest$quantile) == "50", + ] + if (nrow(median_df) >= 1) { + PEcAn.logger::logger.debug( + "Using shared median run for", trait, "quantile 50" + ) + run.id <- utils::tail(median_df$run_id, 1) + } else { + PEcAn.logger::logger.warn("No run found in manifest for", trait, quantile) + next # Skip this quantile + } } else { PEcAn.logger::logger.warn("No run found in manifest for", trait, quantile) next # Skip this quantile diff --git a/modules/uncertainty/tests/testthat/test-read_sa_output_median.R b/modules/uncertainty/tests/testthat/test-read_sa_output_median.R new file mode 100644 index 00000000000..4723ce5a64f --- /dev/null +++ b/modules/uncertainty/tests/testthat/test-read_sa_output_median.R @@ -0,0 +1,99 @@ +# Regression test for the shared-median manifest fallback in read.sa.output() +# +# Background: write.sa.configs() writes the median (q50) run as a single shared +# entry with pft_name = "NA" and trait = "NA". read.sa.output() previously did +# a strict per-trait/per-PFT lookup, so the median row always failed to match +# and the 50th-quantile cell was returned as NA. +# +# The fix adds a fallback: when quantile == "50" and no exact match is found, +# look for the shared median entry before giving up. +# +# Reference: https://github.com/pecanproject/pecan/issues/ + +test_that("read.sa.output resolves median (q50) via shared manifest fallback", { + withr::with_tempdir({ + pft <- "temperate.coniferous" + trait <- "growth_resp_factor" + yr <- 2004 + + # ---- Build a manifest that matches what write.sa.configs() actually writes ---- + # One shared median row (pft_name = "NA", trait = "NA") + no trait-specific row + # for quantile 50. This is the exact situation that triggered the original bug. + median_run_id <- "SA-median--1" + manifest <- data.frame( + type = "Sensitivity", + pft_name = "NA", + trait = "NA", + quantile = "50", + run_id = median_run_id, + stringsAsFactors = FALSE + ) + write.csv(manifest, "runs_manifest.csv", row.names = FALSE) + + # ---- Stub out the model output directory so read.output() finds a file ---- + run_outdir <- file.path(getwd(), median_run_id) + dir.create(run_outdir, recursive = TRUE) + + # Create a minimal NetCDF file with an NPP variable for the target year + nc_path <- file.path(run_outdir, paste0(yr, ".nc")) + nc_obj <- ncdf4::nc_create( + nc_path, + list(ncdf4::ncvar_def("NPP", "kg m-2 s-1", list(), missval = NA_real_)) + ) + ncdf4::ncvar_put(nc_obj, "NPP", 1.23) + ncdf4::nc_close(nc_obj) + + # ---- Run read.sa.output() ---- + # Should NOT warn "Run ID invalid or missing" for quantile 50 any more. + expect_no_warning( + out <- PEcAn.uncertainty::read.sa.output( + traits = trait, + quantiles = "50", + pecandir = getwd(), + outdir = getwd(), + pft.name = pft, + start.year = yr, + end.year = yr, + variable = PEcAn.utils::convert.expr("NPP") + ), + regexp = "Run ID invalid or missing" + ) + + # The median cell must not be NA + expect_false( + is.na(out[["50", trait]]), + label = "median (q50) output should not be NA" + ) + }) +}) + + +test_that("read.sa.output still warns when no median fallback row exists", { + withr::with_tempdir({ + # Manifest with quantile 50 row that has the wrong pft_name (not "NA"), + # meaning neither exact match nor shared-median fallback can be found. + manifest <- data.frame( + type = "Sensitivity", + pft_name = "some.other.pft", + trait = "NA", + quantile = "50", + run_id = "SA-median--1", + stringsAsFactors = FALSE + ) + write.csv(manifest, "runs_manifest.csv", row.names = FALSE) + + expect_warning( + PEcAn.uncertainty::read.sa.output( + traits = "growth_resp_factor", + quantiles = "50", + pecandir = getwd(), + outdir = getwd(), + pft.name = "temperate.coniferous", + start.year = 2004, + end.year = 2004, + variable = PEcAn.utils::convert.expr("NPP") + ), + regexp = "No run found in manifest" + ) + }) +}) From 451019de5e646639272ecd6d6c600ef6dbea6df3 Mon Sep 17 00:00:00 2001 From: David LeBauer Date: Wed, 8 Apr 2026 21:42:09 -0700 Subject: [PATCH 2/4] Update test-read_sa_output_median.R --- modules/uncertainty/tests/testthat/test-read_sa_output_median.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/uncertainty/tests/testthat/test-read_sa_output_median.R b/modules/uncertainty/tests/testthat/test-read_sa_output_median.R index 4723ce5a64f..82854345711 100644 --- a/modules/uncertainty/tests/testthat/test-read_sa_output_median.R +++ b/modules/uncertainty/tests/testthat/test-read_sa_output_median.R @@ -8,7 +8,7 @@ # The fix adds a fallback: when quantile == "50" and no exact match is found, # look for the shared median entry before giving up. # -# Reference: https://github.com/pecanproject/pecan/issues/ +# Reference: https://github.com/pecanproject/pecan/issues/3882 test_that("read.sa.output resolves median (q50) via shared manifest fallback", { withr::with_tempdir({ From 29141f575c2fde58f5300e2ff6f8996a8db2e5d1 Mon Sep 17 00:00:00 2001 From: Man Date: Sat, 11 Apr 2026 09:45:41 +0530 Subject: [PATCH 3/4] fix(tests): correct variable argument and add ncdf4 to Suggests The regression test for issue #3882 had two bugs: 1. The test passed PEcAn.utils::convert.expr('NPP') directly as the ariable argument to read.sa.output(), but the function expects only the \.eqn sub-list (containing \ and \). The full convert.expr() output has those fields one level deeper (\.eqn\), so variable\ and variable\ were both NULL, causing read.output() to fail silently. Fix: append \.eqn to both call sites, matching how get.results.R passes the value in production. 2. The test creates real NetCDF files with ncdf4::nc_create() etc., but ncdf4 was not listed in DESCRIPTION Suggests, so it would not be installed in CI. Fix: add ncdf4 to Suggests, and add the corresponding row to pecan_package_dependencies.csv so the 'check for out-of-date dependencies files' CI step stays green. Also replace the placeholder '' with 3882 in the test file comment. Fixes #3882 (follow-up) --- docker/depends/pecan_package_dependencies.csv | 1 + modules/uncertainty/DESCRIPTION | 1 + .../uncertainty/tests/testthat/test-read_sa_output_median.R | 4 ++-- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docker/depends/pecan_package_dependencies.csv b/docker/depends/pecan_package_dependencies.csv index 1c64dd7120a..40f5570919e 100644 --- a/docker/depends/pecan_package_dependencies.csv +++ b/docker/depends/pecan_package_dependencies.csv @@ -246,6 +246,7 @@ "ncdf4","*","models/stics","Imports",FALSE "ncdf4","*","modules/assim.sequential","Imports",FALSE "ncdf4","*","modules/data.remote","Imports",FALSE +"ncdf4","*","modules/uncertainty","Suggests",FALSE "ncdf4",">= 1.15","base/utils","Imports",FALSE "ncdf4",">= 1.15","base/visualization","Imports",FALSE "ncdf4",">= 1.15","models/biocro","Imports",FALSE diff --git a/modules/uncertainty/DESCRIPTION b/modules/uncertainty/DESCRIPTION index d9577316963..1ab0f4a1c9a 100644 --- a/modules/uncertainty/DESCRIPTION +++ b/modules/uncertainty/DESCRIPTION @@ -46,6 +46,7 @@ Imports: sensitivity Suggests: mockery, + ncdf4, testthat (>= 1.0.2), withr License: BSD_3_clause + file LICENSE diff --git a/modules/uncertainty/tests/testthat/test-read_sa_output_median.R b/modules/uncertainty/tests/testthat/test-read_sa_output_median.R index 82854345711..a7eb8049aa3 100644 --- a/modules/uncertainty/tests/testthat/test-read_sa_output_median.R +++ b/modules/uncertainty/tests/testthat/test-read_sa_output_median.R @@ -54,7 +54,7 @@ test_that("read.sa.output resolves median (q50) via shared manifest fallback", { pft.name = pft, start.year = yr, end.year = yr, - variable = PEcAn.utils::convert.expr("NPP") + variable = PEcAn.utils::convert.expr("NPP")$variable.eqn ), regexp = "Run ID invalid or missing" ) @@ -91,7 +91,7 @@ test_that("read.sa.output still warns when no median fallback row exists", { pft.name = "temperate.coniferous", start.year = 2004, end.year = 2004, - variable = PEcAn.utils::convert.expr("NPP") + variable = PEcAn.utils::convert.expr("NPP")$variable.eqn ), regexp = "No run found in manifest" ) From b5c859fbe2df9b5df57999cd268f11d69a2f4ef0 Mon Sep 17 00:00:00 2001 From: Man Date: Wed, 15 Apr 2026 13:35:42 +0530 Subject: [PATCH 4/4] fix: use is.na() and which() for median manifest fallback lookup When write.sa.configs() writes the shared median entry with pft_name='NA' and trait='NA', read.csv() converts these string 'NA' values to actual R NA values. This caused two problems in read.sa.output(): 1. The initial exact-match filter used == comparisons, which return NA (not FALSE) when comparing with NA values. Subsetting with NA indices produces phantom rows, so nrow(subset_df) > 0 and the median fallback branch was never reached. Fixed by wrapping with which(). 2. The median fallback filter compared manifest == 'NA' (string), which also returns NA when the column contains actual R NA. Fixed by using is.na() instead. Fixes #3882 --- modules/uncertainty/R/sensitivity.R | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/modules/uncertainty/R/sensitivity.R b/modules/uncertainty/R/sensitivity.R index 3370fc8b6e3..3420b5d00c5 100644 --- a/modules/uncertainty/R/sensitivity.R +++ b/modules/uncertainty/R/sensitivity.R @@ -38,11 +38,15 @@ read.sa.output <- function(traits, quantiles, pecandir, outdir, pft.name = "", for (trait in traits) { for (quantile in quantiles) { # We look for the row that matches the current pft, trait, and quantile. + # Use which() to handle NA values in manifest columns (read.csv converts + # the string "NA" to actual R NA, and comparison with NA yields NA, not FALSE). subset_df <- manifest[ - manifest$type == "Sensitivity" & - manifest$pft_name == pft.name & - manifest$trait == trait & - as.character(manifest$quantile) == as.character(quantile), + which( + manifest$type == "Sensitivity" & + manifest$pft_name == pft.name & + manifest$trait == trait & + as.character(manifest$quantile) == as.character(quantile) + ), ] if (nrow(subset_df) == 1) { @@ -53,10 +57,12 @@ read.sa.output <- function(traits, quantiles, pecandir, outdir, pft.name = "", } else if (as.character(quantile) == "50") { # The median run is written as a single shared entry with pft_name = "NA" # and trait = "NA". Fall back to that shared row when no exact match exists. + # Note: read.csv() converts the string "NA" to actual R NA values, + # so we must use is.na() rather than == "NA" for the comparison. median_df <- manifest[ manifest$type == "Sensitivity" & - manifest$pft_name == "NA" & - manifest$trait == "NA" & + is.na(manifest$pft_name) & + is.na(manifest$trait) & as.character(manifest$quantile) == "50", ] if (nrow(median_df) >= 1) {