fix(uncertainty): resolve SA median manifest lookup (#3882) - #4096
Open
ANAMASGARD wants to merge 5 commits into
Open
ANAMASGARD wants to merge 5 commits into
ANAMASGARD wants to merge 5 commits into
Conversation
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 PecanProject#3882
The regression test for issue PecanProject#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 '<issue-number>' with 3882 in the test file comment. Fixes PecanProject#3882 (follow-up)
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 PecanProject#3882
expect_no_warning() no longer accepts regexp in testthat 3.x, and PEcAn.logger::logger.warn() does not emit R warnings. Use expect_output and NA result checks instead, matching test.load_posteriors.R. Fixes PecanProject#3882 (follow-up) Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3882 —
read.sa.output()was dropping the median (q50) row when using the manifest-based OAT sensitivity path.Supersedes #3906 (same production fix; adds corrected regression tests that pass CI). Builds on work by @man080107 with test fixes for testthat 3.x and PEcAn.logger.
Motivation and Context
write.sa.configs()/sa_run_descriptions()write one shared median run withpft_name = "NA"andtrait = "NA". Afterread.csv(), those become RNA.read.sa.output()previously required an exact per-trait/per-PFT match (and==comparisons withNAfail silently), so the q50 cell was alwaysNAand downstream sensitivity analysis proceeded without a valid median response.Per @dlebauer in #3882, this PR implements option (2): keep the shared median manifest entry and add a read-side fallback when
quantile == "50".Changes
modules/uncertainty/R/sensitivity.R— NA-safe exact match viawhich(); median fallback viais.na()on shared rowmodules/uncertainty/tests/testthat/test-read_sa_output_median.R— regression tests usingexpect_output(PEcAn.logger pattern fromtest.load_posteriors.R)modules/uncertainty/NEWS.md,DESCRIPTION,docker/depends/pecan_package_dependencies.csv—ncdf4SuggestsReview Time Estimate
Types of changes
Checklist
devtools::test(..., filter='read_sa_output_median'); full CI pending)Test plan
devtools::test('modules/uncertainty', filter='read_sa_output_median')— PASS locallytest (4.2)/test (4.4)Labels requested
modules,tests,bug