Update demos 1 and 2 to build the design explicitly - #4063
Open
omkarrr2533 wants to merge 16 commits into
Open
Conversation
…data generate_OAT_SA_design now samples in memory via load_pft_posteriors and get_parameter_samples, requesting SA quantiles without an ensemble draw, and returns list(X, samples) like the ensemble generator. Its sa_samples argument becomes samples, taking the full bundle so both generators are called the same way. .prepare_input_designs threads the resolved bundle into it, and warns that internal design generation is deprecated and passing input_design will become required. Add tests for the in-memory sampling, the pass-in path, the empty sa.samples guard, and the deprecation warning.
generate_joint_ensemble_design already samples before it builds the sobol matrices, but returned the soboljansen object without them, so a sobol design could not be handed to the config writers. Attach the bundle as samples, leaving the components sensitivity::tell() uses untouched, and update the sobol example script to pass the whole object. Add tests covering the samples being carried, reused when passed in, drawn for the doubled ensemble, and tell() still working afterwards.
Both generators now return the design under design_matrix, which says what it is, and keep X so existing callers are unaffected. On a sobol design X is set by sensitivity itself, so design_matrix is attached alongside it and the two return shapes read the same way. .prepare_input_designs reads design_matrix and falls back to X, both for a supplied design and for one it generates, so a mixed-version install keeps working in either direction. The design stays the first element of the return because sda.enkf_MultiSite and sda.enkf_parallel take it positionally, and a test locks that. Docs on both generators are also brought back in line with what they actually do since they stopped touching samples.Rdata.
Since PecanProject#4054 the design and its samples have to arrive together, so passing sens_design\ on its own now errors. The other uses of \ here still want the matrix itself and are unaffected.
Both demos called runModule.run.write.configs(settings) and let it generate the design internally, which is the path being deprecated. They now call generate_joint_ensemble_design() first and pass the whole result, which is also what a user's own workflow should look like. Demo 2 exposed a gap: it runs a sensitivity analysis as well, and there is no way to supply an SA design, so that one is still generated internally. The deprecation warning now only fires for the design actually being generated, rather than telling a caller who did supply one to supply it again. Also adds the roxygen blank line that keeps the generator's title from swallowing its description.
…or them The generator hardcoded sa_quantiles = NULL while .prepare_input_designs sampled with settings\.analysis\, so the same settings produced different bundles depending on which one did the sampling. That was hidden until a supplied bundle became authoritative: since then a run with both an ensemble and an SA, following the documented pattern of generating the design first, handed over a bundle with no sa.samples and the OAT generator stopped with 'sa.samples are empty'. Rendering demo 2 is what surfaced it. Settings without a sensitivity analysis are unaffected, since the quantiles are NULL there anyway.
mdietze
requested changes
Aug 7, 2026
| ensemble_size = settings$ensemble$size | ||
| ) | ||
|
|
||
| settings <- PEcAn.workflow::runModule.run.write.configs( |
Member
There was a problem hiding this comment.
per comments on Slack, I think this module will need a substantial rewrite if we're going to illustrate both a SA design and an ensemble design. That said, Demo 1 already does an ensemble design (it just defaults to n=1) so another option, which I think may be preferable, would be to have Demo 1 increase its ensemble size (e.g. n=10), and update the visualizations to show all the ensemble members, and then Demo 2 can focus solely on doing a SA run with visualizations for SA and uncertainty partitioning.
Member
Author
There was a problem hiding this comment.
done for demo 1, it's ten members now with every member drawn on each plot, and the notebook render is green.
demo 2 going SA-only i've left out of this PR. as things stand whatever you pass as input_design becomes the ensemble design and the SA config-writing branch only fires off designs$sensitivity, so calling the OAT function first would put it in the wrong slot. that's the two-separate-calls change from slack, so demo 2 will come with that rather than be half-done here.
Brings back the settings requirements list, the OAT design logic explanation and the worked example, plus the inline comments on how the run count is derived and why every non-param column is held at 1. Left out the old note on internal dependencies and the samples.Rdata output directory, since the function no longer calls get.parameter.samples or touches that file.
Member
Author
|
demo 1 with a small ensemble and updated visualizations is coming here next. demo 2 going SA-only needs the design routing to change first, so that'll come with the two-separate-calls PR rather than being half-done here. |
Demo 1 ran a single realisation, so the plots were one line each and nothing about the ensemble machinery was visible. It now runs ten members, reads all of them rather than just the first run id, and each plot draws one line per member so the parameter spread carried through the model is what you see.
omkarrr2533
marked this pull request as ready for review
August 8, 2026 09:38
Ten members overlaid on sub-daily GPP and NPP filled the panel solidly, so the flux plot now averages each member to daily values first. LAI and above ground wood were sharing one panel on very different scales, which flattened LAI, so they are plotted separately. Notes the reason snow water equivalent shows no spread: it follows the met input, which every member shares.
Some members run negative NPP through the growing season, which looks wrong without context. It follows from sampling priors rather than a meta-analysis posterior, so the note says so and points at demo 3.
Went through the whole diff for comments removed during the migration. Most were describing the old get.parameter.samples and samples.Rdata behaviour, which would now be wrong, or were reworded for the design_matrix rename. This one was a real loss: the list explaining how each PFT's posterior is resolved had been folded into prose. It is still accurate, so it goes back as a list.
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.
Stacked on #4060. Per @mdietze on Slack, updating the demos to the dash functions so new users see the current shape, old users see what to change, and the rendered notebooks act as an integration test.
what changed
Both demos called
runModule.run.write.configs(settings)and let it generate the design internally, which is the path being deprecated. They now callgenerate_joint_ensemble_design()first and pass the whole result, which is what a user's own workflow should look like. The surrounding text explains what the design actually is and thatdesign$design_matrixanddesign$samplescan be inspected before any runs happen.demo 1 as an ensemble
Per @mdietze's review, Demo 1 now runs ten members instead of one. It reads every run id rather than just the first, and each plot draws one line per member, so the parameter spread carried through the model is visible instead of a single line. Rendering it in CI runs all ten, which takes about the same time as before.
Demo 2 is migrated but not yet restructured to be SA-only. That needs the design routing to change first, since anything passed as input_design currently lands in designs$ensemble while the SA branch reads designs$sensitivity. It'll come with the two-separate-calls PR.
what rendering demo 2 caught
Demo 2 didn't just warn, it failed.
generate_joint_ensemble_design()hardcodedsa_quantiles = NULLwhen sampling, while.prepare_input_designssamples withsettings$sensitivity.analysis$quantiles. Same settings, different bundles depending on which one sampled. That was invisible until #4054 made a supplied bundle authoritative: after that, any run with both an ensemble and an SA that followed the documented pattern handed over a bundle with nosa.samples, and the OAT generator stopped with "sa.samples are empty".The generator now samples the quantiles from the settings, so both paths produce the same bundle. Settings without a sensitivity analysis are unaffected, since the quantiles are NULL there anyway. Demo 2 is now fully migrated.
Worth noting this is exactly what @mdietze wanted the demos for: no unit test caught this, rendering the notebook did.
testing
The workflow tests cover the narrowed warning: it fires when the ensemble design is generated, and stays quiet when the caller supplied one, including when a SA is configured. Both .qmd files pass the styler check the notebook workflow runs.