fix(uncertainty): resolve SA median manifest lookup (#3882)#3906
Open
man080107 wants to merge 3 commits intoPecanProject:developfrom
Open
fix(uncertainty): resolve SA median manifest lookup (#3882)#3906man080107 wants to merge 3 commits intoPecanProject:developfrom
man080107 wants to merge 3 commits intoPecanProject:developfrom
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
Contributor
Author
|
@dlebauer!!! please do quick review on it.... |
6 tasks
dlebauer
approved these changes
Apr 9, 2026
Member
dlebauer
left a comment
dlebauer
left a comment
There was a problem hiding this comment.
Logic looks right to me. The fallback matches how write.sa.configs() stores the shared median run, and the regression tests test the failure mode.
Thank you for fixing this!
modules/uncertainty/tests/testthat/test-read_sa_output_median.R
Outdated
Show resolved
Hide resolved
Member
|
@man080107 Please check test failures |
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)
auto-merge was automatically disabled
April 11, 2026 04:17
Head branch was pushed to by a user without write access
Contributor
Author
|
@dlebauer !! now i think it is ready to merge |
Member
|
@man080107 it is still failing one required test https://github.com/PecanProject/pecan/actions/runs/24274463741/job/70885636636?pr=3906 |
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
2 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.
Summary
Fixes #3882 -
read.sa.output() was silently dropping the median (q50) run when using the manifest-based OAT sensitivity path.
Root Cause
write.sa.configs() writes a single shared median run with pft_name = "NA" and rait = "NA" in the manifest. However,
ead.sa.output() performed an exact per-trait/per-PFT lookup, so no row ever matched for the median quantile - the result was always NA, and the median was excluded from downstream sensitivity analyses.
Fix
Added a fallback in
ead.sa.output(): when quantile == "50" and no exact match is found, the function now searches for the shared NA/NA median entry before emitting a warning.
Changes
Testing
New tests added in est-read_sa_output_median.R covering both the happy path (fallback resolves correctly) and the error path (warning emitted when no match at all).