Skip to content
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

Array Broadcasting Semantics for BMI Formulations #551

Open
hellkite500 opened this issue Jun 27, 2023 · 4 comments
Open

Array Broadcasting Semantics for BMI Formulations #551

hellkite500 opened this issue Jun 27, 2023 · 4 comments

Comments

@hellkite500
Copy link
Member

hellkite500 commented Jun 27, 2023

As grid support is being implemented for BMI modules, it is now possible for a shape mismatch to occur between variables. This triggered a bug identified in #532 and a fix implemented #534. Similarly, in #549 the issue was related to the relationship between an input variable being scalar and the corresponding variable it was mapped to being an array variable.

The following fix, implemented in #534, is the minimal required to avoid the issue, and it involves simply "broadcasting" the scalar value to each of the array elements before passing that array to the bmi set_value function.
in set_model_inputs_prior_to_update, when varItemSize != get_bmi_model()->GetVarNbytes(var_name) we check the number of values from the data provider, and if it is scalar, we assign that value to the entire array.

if(values.size() == 1){
    //FIXME this isn't generic broacasting, but works for scalar implementations
    values.resize(nbytes/varItemSize, values[0]);
}

This implementation alone isn't sufficient, and needs some additional consideration.
The following are some options, in order of complexity and utility

  1. Write the else case to throw an error
  2. Write warning when broadcast is performed
  3. Allow a configuration option to turn warning into error
  4. Make broadcast support applicable per module input/output

The first is essential unless/until we want to provide full multi-dimensional broadcasting support, which I don't foresee being a priority for some time. We should probably also extend the checks to see if variable shapes match, but any other case should be an error.

The second step, and easy enough to include with the first, is to simply write/log a warning when scalar broadcast in performed, in case that wasn't the desired semantics of the modeler, they are at least aware that it is happening.

The third option would be to provide a global config variable that would allow this type of broadcast (perhaps with/without the warning), but also an option to turn that warning/case into an error that would stop the model run.

Finally, a per-module flag to explicitly enable this behavior could be added to fine-tune the possibility of scalar broadcasting. IMHO this puts a lot of burden on modelers to understand the inner details of a set of potential modules that they could learn from comprehensive reporting from the framework without having to dig into module source code, so I'm not sure this is a good idea, but it is a possible one.

@PhilMiller
Copy link
Contributor

A stray thought: the degree of automation and/or alerting could in part be conditioned on the units/dimensionality of each variable in question. For instance, intensive quantities might be less questionable to map from a scalar (presumably describing a point value of a lumped system) to an array (presumably describing a grid of points across a corresponding area).

@PhilMiller
Copy link
Contributor

The variables that brought this up in #556 were all generic GRID_VAR_1, but given that for actual application usage, we have a fairly extensive ontology and enumeration of variables that may appear in the interfaces of the various models, we could potentially construct a whitelist of variables that are 'safe' to broadcast (along with their expected physical dimensions, as a guard-rail). As usage expands and the set of models people couple through ngen grows, we can organically expand that list.

@PhilMiller
Copy link
Contributor

On a separate stream of thought, maybe we're thinking about addressing this in the wrong place, and this should actually be addressed by the spatial coupler, with the ngen engine itself being strict about what it passes between models

@mattw-nws
Copy link
Contributor

this should actually be addressed by the spatial coupler

This is probably correct. If simple "broadcasting" behavior is desired, this should probably be explicitly chosen by some kind of configuration for the spatial coupler between layers of different discretizations. Outside of that case, e.g. in a multi-BMI formulation running in a single layer/single discretization, it is probably the right answer to just fail if a scalar output is linked to a non-scalar input or vice-versa, or basically any other kind of shape mismatch. If this kind of scenario is something we want to support, we probably need to identify the use-case beyond just a synthetic development/testing situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants