-
Notifications
You must be signed in to change notification settings - Fork 78
Implementing a BMI mass balance checking protocol #909
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
…ce protocol semantics
1570913 to
7d5c585
Compare
|
Force pushed to fix failing bmi multi test (was missing a test config file). MacOS tests seem to be queued indefinitely, but I'll note that I ran tests locally on macos. |
|
Just adding to the above description. I'm probably off a little here, so please correct me where i'm wrong :). For a bmi module to comply with the mass balance protocol a module must expose the following "variables" to These variables must be available to
|
c2da798 to
4267f42
Compare
ca449fa to
8777659
Compare
0dc6ed3 to
ddc4c2b
Compare
| -DNGEN_WITH_SQLITE:BOOL=${{ inputs.use_sqlite }} \ | ||
| -DNGEN_WITH_MPI:BOOL=${{ inputs.use_mpi }} -S . | ||
| -DNGEN_WITH_MPI:BOOL=${{ inputs.use_mpi }} \ | ||
| -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -S . |
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.
This is here to make pybind's cmake happy. See pybind/pybind11#5593
| - name: Run Unit Tests | ||
| run: | | ||
| . .venv/bin/activate | ||
| export ASAN_OPTIONS=${ASAN_OPTIONS}:detect_odr_violation=0 |
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.
See 08d8379 for explanation.
ddc4c2b to
89abd58
Compare
3109068 to
c3c1a80
Compare
|
The last couple commits here have all been around trying to ensure the address sanitizer on macos using Clang 17 is behaving as expected by
Despite these efforts, I cannot get a clean bill of health for the a few of the macos tests cases, and they seem to be indeterminate. test_bmi_c all fail, but they don't always fail, with our without the previous changes. One final thing we may want to try is to use a dynamic linked asan instead of static link (default for clang) which some indications say may help avoid the (possibly) false positives coming form the linked/dlopened libs. Since we don't see the gcc sanitizer on ubuntu runners complaining, I'm inclined to stop pushing to this PR trying to fix up all the CI and move that its own issue/PR and let this merge. @aaraney Thanks for the reviews, sorry about invaliding them so many times. One more for good measure? |
c3c1a80 to
4a5b319
Compare
|
Canceled the testing and validation workflow after the ubuntu tests ran as the macos tests will never run and will evenutally timeout anyways. #913 looks to address the mac runners. |
aaraney
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.
Looks great! Thanks for working through this, @hellkite500!
| auto NgenMassBalance::initialize(const ModelPtr& model, const Properties& properties) -> expected<void, ProtocolError> | ||
| { | ||
| //Ensure the model is capable of mass balance using the protocol | ||
| check_support(model).or_else( error_or_warning ); |
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.
Coming back to this, I think we should check model support only after reading the configuration. Otherwise, this will raise an exception for models that don't support the mass balance checker and aren't configured.
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.
fixed in #916
This feature allows the model engine to attempt dynamic mass balance introspection of BMI modules via a specified
protocolwhich dictates the BMI variables names representingcumulativemass balance variables which can be inspected at any point afterbmi_intialize()is called.It is assumed that these variables balance mass within the model, e.g.
where
mass_inis the model's cumulative mass in (e.g. precipitation)mass_outis the model's cumulative mass out (e.g. overland flow)mass_storedis the model's currently stored mass (e.g. soil moisture reservoirs)mass_leakedis cumulative mass leaked from the domain which is known loss to the model (e.g. deep groundwater)When
balanceis greater than a configurable threshold (provided as a formulation parameter in the realization file) then this either prints a warning or throws an error, based on the configuration.Additions
Configurable mass balance protocol implementation. The follow object definition in a formulations
paramsobject is used to configure the mass balance implementation.Where
fatal, boolWhen true, a fatal exception is thrown when mass balance tolerance is violated.
When false, a mass balance warning capturing details of the error is printed, but the simulation continues.
tolerance, floatHow far from 0 the mass balance can get before it is considered an error, i.e.
balance > tolerancewill trigger a mass balance error/warning.check, boolToggle the mass balance checking on or off
frequency, intHow often, in number of timesteps, to check the mass balance in the model engine. If frequency is set to -1, then it will only be checked at the last time step.
Changes
boost>=1.86.0check_mass_balanceupon every update.Testing
Notes
Todos
to occur if the mass balance units aren't all the same, but no conversion is attempted.
Checklist
Target Environment support