Improve unit conversion error reporting#959
Conversation
…ce data_access to support CSV and NetCDF reporting
…than printing locally
…used in Bmi_Multi_Formulation testing
…rather than printing locally
…ic units in tests, and disable now-throwing non-conversion cases
2bd6aee to
1ffa31f
Compare
hellkite500
left a comment
There was a problem hiding this comment.
While I support the general idea behind this, I have some design and implementation concerns that may require a bit of dialogue and/or refactoring to get through.
|
All useful feedback. I'll try to address it shortly. |
robertbartel
left a comment
There was a problem hiding this comment.
FWIW, I'm not fully done with review - still trying to make sure I have my head wrapped around how this executes, especially any differences with scalar versus vector values. But I did want to go ahead and make a few initial comments.
The only clear thing at this point that I can say needs changing is that the BMIconventions.md document needs to be updated to reflect these behavior changes, especially related to unitless and none-ish conventions.
A couple other early impressions that aren’t fully formed yet, but might stimulate some useful discussion …
- This feels a little brittle. In fairness, that’s probably got a lot to due with the larger design of things around it, although this doesn’t necessarily disqualify the observation from relevance.
- Throwing an(other) exception on every variable read with units issues seems like it could get a little computationally expensive. Cheaper than the IO overhead of
cerr-ing every time a warning condition is encountered, but that could be turned off. It doesn’t look like this exception-based approach can be.
|
My experience with these changes on the NGWPC side is that they ultimately pushed the overall system toward an overall more robust situation, but there was a lot of reconciliation of these warning messages along the way. There's a follow-on PR coming that also optionally specifies the desired units of output variables named in the realization config, which would avoid modeling misinterpretations and a current inconsistency between mosaiced formulations for different catchments that output the same variables but in different units. Maybe I'll put that up as a PR against this branch, so we can see and evaluate it in context. |
|
Regarding the exceptions concern, the actual exception |
Hydrologic simulation in NGen is driven by Layer calling catchment_formulation->get_response() on each catchment's formulation instance in turn. The get_response() method served two related but distinct roles: 1. Advancing the simulation by one step within a formulation or one element thereof 2. Calculating and returning the hydrologic response from that step Within a Bmi_Multi_Formulation, the latter responsibility was only applicable to modules actually providing the runoff variable, and not others that generated inputs to that module or ancillary output variables. However, get_response() was the means to advance them in time, and their primary output variable had to be queried accordingly. Advancing in time is now split out to an update() member function(). By making that split, get_response() can be updated to consistently return results in the units of "m" (depth) expected by the caller in Layer::update_models(). Without the associated changes, every other module would incur spurious unit conversion errors at every time step, when asked for some arbitrary 'main output variable' via get_response() that may be in other units.
|
I haven't directly addressed the review comments yet, but I did push an additional commit that better applies and motivates these changes. That change could potentially be made on its own, but it kinda comes with the rest of what's here, so that's how I'm presenting it at the moment. If asked, I could propose that in more isolation instead or in advance. |
|
On the string passing overheads in the error path, I think I'm going to push back a little bit on trying to change that right away. I agree it could become a burden. However, a 'healthy' simulation run that's not incurring unit mismatches will never incur that burden. For pre-production (benchmarking, calibration, regionalization, retrospective, etc) and operational use cases, where we're most concerned about performance, we should have hammered out those issues before running. For a scientifically meaningful run, I really don't think there's an excuse for accepting unit mismatches. If we need some targeted adapters for (e.g.) More broadly, we need to profile and see if/where passing around so many strings actually does cost ngen performance, and start grinding those away. |
687c078 to
a97c44a
Compare
a97c44a to
c55c433
Compare
…olidated into UnitsHelper
Improve how errors in unit conversion are tracked and reported. Each particular mis-match between distinct models and their variables will be logged exactly once. The reports have comprehensive detail allowing for straightforward correction of the underlying modules or configuration.
Additions
unit_conversion_exceptiontype ("UCE") when UDUNITS fails to apply a conversion, storing the provider and variable detailsChanges
update()from the existingget_response(), which is now specifically used as getting the effective depth of surface runoffTesting
Notes
Checklist