-
Notifications
You must be signed in to change notification settings - Fork 274
Partial fixes for multisite ensembles #3654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@DongchenZ Will anything I propose here conflict with #3634? |
|
LGTM, as discussed my debug shows the settings will get mutated between two calls of |
@divine7022 Good catch that those are potential failure points. On a quick look I think their current behavior (of erroring if no ensemble id) is correct -- recall that the model runs have already executed by the time run.ensemble.analysis and read.ensemble.ts is called, so if we can't tell what ensemble ID was used at write.configs time then I think it's too late to create one in these steps. Does that seem right to you? |
|
sounds correct!, realizing the different ensemble.id validation behaviors for sensitivity and ensemble runs ; and strict ensemble.id requirements for |
dlebauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems prudent to merge this hack to keep work moving. But before merging, please file one or more follow-up issues to track the open threads that it leaves.
Co-authored-by: David LeBauer <[email protected]>
@dlebauer this PR fixes the problem I was having, so as far as I'm concerned it's not leaving threads open 🤷 Like I said above, the dump of my working notes is specifically for you to take and run with, because the correct resolution for each "unresolved" behavior (or even the decision whether it's a bug at all) could depend greatly on which direction you decide to take the SA work. |
|
Those fixes are addressed and will be pushed once this PR gets merged |
dlebauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1901bbe
Description
A couple small fixes and a questionable hack for running ensemble and uncertainty analyses without database access.
SA-median/isn't overwritten for each site in turnNote that the latter step is done independently for each call to
write.*.configs, so in a multisite run this will effectively set up a separate ensemble/SA for each site. This was what I wanted today, but I suspect most people will want outputs aggregated across sites, which this PR does not implement.Motivation and Context
For the MAGiC project I wanted to quickly evaluate AGB timeseries from many sites, for which the timeseries plots from the ensemble analysis would be perfect except that I'm running with no Bety access and the existing code sets the ensemble ID to
NOENSEMBLEID, making each site overwrite the outputs from the previous one.Since the issue applies to both ensemble and sensitivity I tried to implement a fix for both, but note that I focused on avoiding collisions between distinct ensembles -- there are still places where two sites with the same ensemble ID will overwrite each other.
I'm pasting my working notes below -- @divine7022 and @dlebauer will likely want to consider the unresolved issues in their work on multisite sensitivity.
write.configs fails if SA is requested in a settings with ensemble size > 1
=> unresolved
(minor): README.txt does not specify which met/IC/soil/event inputs were used
=> unresolved
rundir
SA-<pft>-<var>-<quantile>contents are overwritten by each site in turn=> Resolved by adding site id to the get.run.id call
rundir SA-median- tries to run analysis for "ALL PFT", fails on NAs from pfts not present at that site
=> Resolved by having run.sensitivity.analysis subset PFTs to those in run$site$site.pft. PFT doesn't show up in rundir names, but since only one per site it works.
each site's call to run.write.configs overwrites
sensitivity.samples(But I think this is where run IDs are taken from)
=> Unresolved
runModule.run.sensitivity.analysis overwrites outputs as it runs for each site
=> This workaround implemented by setting null ensemble.ids to
rlang::hash(settings),but if we might consider settings$run$site$id instead. Are there cases where a multiSettings might contain multiple entries from the same site? or where site ID would be unset?
Review Time Estimate
Types of changes
Checklist: