Skip to content

Conversation

@blesson-07
Copy link
Contributor

@blesson-07 blesson-07 commented Mar 27, 2025

Issue Title
Fix Input Sampling Validation and Deterministic IC Handling

Problem
PEcAn improperly passed multiple input paths (e.g., initial conditions) to model configuration functions without upstream sampling. Models like SIPNET handled this by randomly selecting a path, violating PEcAn’s design where sampling should occur at the ensemble level. This led to:

Non-reproducible workflows (random IC selection).

Lack of validation for unsampled multi-path inputs.

Solution
Upstream Validation:
write.ensemble.configs now errors if an input has multiple paths but no sampling method ( in XML).
Ensures only one path per input is passed to model configs.

@blesson-07
Copy link
Contributor Author

@mdietze @infotroph
Could you review the input validation changes?

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

Thank you! Overall this is looking very good. I'll leave some notes inline, but the main change I see needed is to put the test file in modules/uncertainty/tests/testthat rather than base/all/[...]. I also see what looks to me like the same check in two places -- is that a true duplicate or am I not seeing the reason both are needed?

changed the file location
@infotroph
Copy link
Member

@blesson07asd Will you be able to return to this soon? Seems like it's in pretty good shape overall.

The biggest issue I see remaining is that ensemble-test.R is now removed entirely -- thanks for taking it out of base/all as requested, but can you please add it back in modules/uncertainty?

@blesson-07
Copy link
Contributor Author

@infotroph Apologies for the delay, I was in the middle of my semester exams, which are now over. I’ll resume work on this and get the remaining changes pushed ASAP. Thanks for your patience and support!

@github-actions github-actions bot added the Models label May 9, 2025
@blesson-07 blesson-07 marked this pull request as ready for review May 10, 2025 05:06
@blesson-07 blesson-07 changed the title [WIP] Fix input sampling validation and SIPNET IC handling Fix input sampling validation and SIPNET IC handling May 10, 2025
@blesson-07

This comment was marked as resolved.

@blesson-07 blesson-07 force-pushed the fix/input-sampling-validation branch from 7f07635 to e4d6adc Compare May 29, 2025 18:14
Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

Not sure why you added rhdf5 to the libraries in data.remote as you don't seem to use it anywhere in your code

@mdietze mdietze added this pull request to the merge queue May 30, 2025
@mdietze mdietze removed this pull request from the merge queue due to a manual request May 30, 2025
@mdietze mdietze enabled auto-merge May 30, 2025 11:24
@mdietze mdietze added this pull request to the merge queue May 30, 2025
Merged via the queue into PecanProject:develop with commit d43f6c7 May 30, 2025
18 of 25 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.

4 participants