Support look-back for nest BMI module input values and bootstrapping of defaults. #346
Conversation
| * to eventually be able to provide. | ||
| * | ||
| * This type allows for deferring reconciling of whether a provider for some data output is available. This is | ||
| * useful for situations when a required provider is not current known, but is expected, and there will be ample |
There was a problem hiding this comment.
is not current known should be is not currently known
| * Note that this type does not alter the behavior of inherited functions, except for | ||
| * @ref get_available_forcing_outputs. That particular function is implemented to only return outputs guaranteed to | ||
| * be provideable by the instance. Instances of this type otherwise defer to the wrapped, backing provider object | ||
| * and its function implementations. This means the backing wrapped object's type control behavior of cases when |
There was a problem hiding this comment.
This sentence is confusing
"This means the backing wrapped object's type control behavior of cases when some unknown value name is given."
I think it is just the subject verb agreement is off.
"This means the backing wrapped object's type controls the behavior of cases when some unknown value name is given."
| // Also, in this implementation, don't count usages until there is a backing provider that can provide this | ||
| auto waits_it = defaultUsageWaits.find(output_name); | ||
| if (waits_it != defaultUsageWaits.end() && isSuppliedByWrappedProvider(output_name)) { | ||
| // A value of 0 should be inferred and cleared from the collection, so ... |
There was a problem hiding this comment.
So what exactly happens with a value of 0???
There was a problem hiding this comment.
@hellkite500, the intent was for there never be a value less than 1 in the defaultUsageWaits map. If we assume this condition when an object is created, then it should hold throughout, as any values equal to 1 before the decrementing step are instead removed from the map entirely.
However, your comment prompted me to double-check and discover that it is possible for the > 0 condition to be violated. There is this constructor:
OptionalWrappedProvider(vector<string> providedOuts, map<string, double> defaultVals, map<string, int> defaultWaits)This validates the keys in defaultWaits (needed for other reasons) but not that the values are all greater than 0.
I could fix this, but first I'd like some opinions on which is the right approach. Instead of what I was originally trying, does it make more sense to do as you had expected and check in the recordUsingDefault function for the <= 0 case? It's slightly less efficient but more intuitive.
| * | ||
| * During nested formulation creation, when a nested formulation requires as input some output expected from | ||
| * soon-to-be-created (i.e., later in execution order) formulation (e.g., in a look-back scenario to an earlier | ||
| * time step), then a deferred provider gets registered with the nested module and has a referenced added to |
There was a problem hiding this comment.
"and has a reference added to"
|
A couple additional thoughts. Should the Would we ever want more than a single time step "look back"? |
|
This didn't make AGU/v0.1.0 but I have a feeling it will be wanted for "MVP" to support SoilFreezeThaw along with #343 . |
@hellkite500, I'm not sure I understand exactly what you are asking, though it does seem as though perhaps this may not sufficiently handle the case when a default is needed but not provided. I'll have to double-check that, but I'd like to make sure I understand what you were suggesting before I do.
Possibly, but that has the potential to be a considerably more significant undertaking. E.g., how far back do we save? How do we control what is saved beyond one time step? Where/how is data being saved? Are we doing anything to ensure we don't exceed storage capacity? Unless there is something specific that is fairly simple/similar and immediately needed, we should handle expanding this functionality separately. If there are any particular use cases that we have need for, we'll also need those details. |
robertbartel
force-pushed
the
f/bmi_multi_past_ts_inputs/main
branch
from
677b510 to
94f07e3
Compare
@hellkite500, I realized after our discussion last week, as I was working on fixing the "problem" with this, that I been wrong it how I remembered this was supposed to work. But I think we need to change things anyway. In short, things were made to support perpetually providing a default value for some output. The 0-value case you originally asked about was to allow this, as was the possibility of the constructor receiving wait values less than 1. Or, stated otherwise, the intent was actually for there never be a value less than 1 in the After our talk, I don't think this should be handled by the framework, so I've modified things to take that out. It would have changed the nature of the type from being a wrapper/proxy to being a data source. Also, |
Making private "wrapped_provider" member protected instead to support extending this into subtypes.
Creating deferred wrapped forcing provider type that doesn't need its backing provider at construction, and can instead be associated with it later.
Adding member collections for tracking deferred wrapped providers and the indices of the associated nested module of each.
Updating Bmi_Multi_Formulation init_nested_module function to support using a deferred wrapped forcing provider (that will later be reconciled) when there is not already a known available data source for something, instead of just throwing an exception.
Adding function init_deferred_associations to Bmi_Multi_Formulation to handle reconciling any created deferred wrapped providers with available data providers (typically nested modules) that were not known at the time the deferred provider had to be created.
Updating Bmi_Multi_Formulation create_multi_formulation to support using deferred wrapped forcing providers by running init_deferred_associations function after all nested modules have been created.
Marking 1-arg constructors explicit.
Move by-value arg rather than double copy to initialization list in constructor for DeferredWrappedProvider.
Changing use of "values" to "outputs" where appropriate.
Add support for this type to override a backed value with a default.
Make OptionalWrappedProvider recordUsingDefault function protected.
Adding initial round of unit tests, along with mock forcing provider implementation for testing purposes.
Making isSuppliedWithDefault function public instead of private.
Adding support for BMI multi formulations to have a nested module configure a data provider that is a later nested module, including supporting adding an initial default value for the input via the realization config.
Refactoring get_value with more comments and better use of other functions for more clear design.
Adjusting the rules for considering the namesake wrapped provider valid by counting the number of outputs provided via a would-be wrapped provider, and only considering one to be valid if this count is greater than zero.
Removing partial/inconsistent support for default usage wait values less that one.
Adding tests to make sure that the value from the backing provider is used directly if no default value was supplied.
Adding Bmi_Multi_Formulation_Test as friend of Bmi_Py_Formulation.
Adding Bmi_Multi_Formulation_Test as friend of Bmi_Fortran_Formulation.
Add custom exception indicating realization config problem.
Refactored functions for creating outer module and nested module realization config sections into more language-integration-specific functions to have it be more clear how things work.
Added example cases for deferred provider setup, along with basic init test, with one example being set to not be able to find anything to satisfy deferred required variable.
Adding tests cases to Bmi_Multi_Formulation_Test to ensure the right number of deferred providers and that deferred providers are properly set when the multi formulation's creation function has finished.
Use custom ConfigurationException if a Bmi_Multi_Formulation cannot properly set a wrapped provider for a deferred provider.
Add tests to Bmi_Multi_Formulation_Test to make sure input values from deferred providers are handled and pass properly.
Minor refactoring in Bmi_Multi_Formulation, along with fixes of a few bugs revealed by recently added tests.
robertbartel
force-pushed
the
f/bmi_multi_past_ts_inputs/main
branch
from
4af16b2 to
6f3024d
Compare
|
Added some test case that were not covered after conversation with @mattw-nws. Also refactored the test class a bit. Plus, rebased against latest |
|
This (unit conversion stderr text removed: Looks like you duplicated test |
|
Merging, but noting possible gotcha: I don't see a test that makes sure reading |
Add support for allowing a nested formulation in a BMI multi formulation to use as a provider a later nested formulation. Previously, only earlier nested formulations could be used like this, where earlier/later is with respect to their listed order in the realization config. Since a later nested formulation's outputs won't have been (re)calculated yet, this effectively is a look-back to the previous time step.
Also, adding support for configuring a default value for nested formulation outputs, including having it be used as the value during the first time step in the aforementioned look-back scenario.