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

Allow passing feature properties to BMI models through model_params #588

Merged
merged 13 commits into from
Aug 25, 2023

Conversation

program--
Copy link
Contributor

@program-- program-- commented Jul 26, 2023

This PR resolves #554 by providing a new value specification for formulation model parameters (model_params).

An external model parameter is described as:

<parameter name>: { "source": <source> [, "from": <mapping>] }

Where the following represent:

  • <parameter name>: the name the BMI model will use
  • <source>: This PR only supports a source of "hydrofabric", but this scaffolds out the possibly of supplying a path to a separate file that can be joined on demand.
  • <mapping>: A mapping name that signifies what the property is named as within <source>

An example of what this could look like is as follows:

{
  "model_params": {
    "area_sqkm": {
      "source": "hydrofabric",
      "from": "area"
    },
    "some_static_param": 3.0,
    "length_km": {
      "source": "hydrofabric"
    }
  }
}

In this example, area and length_km are defined within the hydrofabric feature, and are passed to the BMI model as area_sqkm and length_km, respectively.

Note: notice that length_km does not specify a "from" key, so we assume it is named the same within the hydrofabric feature's properties. If it does not exist within the feature properties, it is ignored, and removed from model_params.

Additions

  • Adds a member function parse_external_model_params to realization::Formulation_Manager that performs the parsing/modification of model_params.
  • Adds a small section of logic to Formulation_Manager::read that checks for a model_params key when iterating the catchment-specific formulations.
  • Adds an additional set of unit tests to the formulation manager tests that verifies this PR's functionality.

Testing

All unit tests pass locally in Release with GCC 13.1.1, Clang 15.0.7, and IntelLLVM 2023.

Todos

  • Parse and modify model_params for global formulations.
  • Add tests for global formulation model_params.
  • Parse and modify model_params for catchment-specific multi-BMI formulations.
  • Add tests for catchment-specific multi-BMI model_params.
  • Parse and modify model_params for global multi-BMI formulations.
  • Add tests for global multi-BMI model_params.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux

@program-- program-- added the enhancement New feature or request label Jul 26, 2023
@program-- program-- self-assigned this Jul 26, 2023
@mattw-nws
Copy link
Contributor

Appears we're not building SLoTH in the workflows yet. While automated testing was one of SLoTH's use cases, the thought was more for integration tests... this one is simple enough, can it get away with using test_bmi_cpp instead?

@program--
Copy link
Contributor Author

test_unit is currently failing since it doesn't look like test_bmi_cpp gets built in that workflow. I think the unit tests for this PR probably need to move into Bmi_Cpp_Formulation_Test.cpp, but I need to make sure that's appropriate first.

@mattw-nws
Copy link
Contributor

test_unit is currently failing since it doesn't look like test_bmi_cpp gets built in that workflow. I think the unit tests for this PR probably need to move into Bmi_Cpp_Formulation_Test.cpp, but I need to make sure that's appropriate first.

I have this fixed in a parallel PR... looks like I need to make a subset of that PR into its own PR anyway. I'll try to get this out today.

@mattw-nws
Copy link
Contributor

#594 should fix the unit test issue.

@program-- program-- marked this pull request as ready for review August 3, 2023 20:33
@program-- program-- marked this pull request as draft August 3, 2023 20:38
@program--
Copy link
Contributor Author

program-- commented Aug 3, 2023

Moving back to draft -- accidentally committed upstream changes from SoilMoistureProfiles (which I'm not even sure how those got pulled on my local side).

EDIT: fixed as of force push to 1ef3d47.

@program-- program-- marked this pull request as ready for review August 3, 2023 20:59
@program-- program-- requested a review from mattw-nws August 3, 2023 20:59
@mattw-nws
Copy link
Contributor

#594 was merged which should remove the need for skipping the test if test_bmi_cpp is not built... recommend rebasing.

@mattw-nws
Copy link
Contributor

mattw-nws commented Aug 25, 2023

Hmm. Fun fact... this worked: (with SLoTH...)

      "model_params": {
          "sloth_2_var_C(1,double,m,node,sloth_1_var_A)": 0.0,
          "sloth_2_var_D(1,double,kg)": 4.0,
          "sloth_2_var_E(1,double,L)": 5.0,
          "area_sqkm(1,double,m^2)": {"source": "hydrofabric", "from": "areasqkm" }
      },

@mattw-nws mattw-nws merged commit 097d34b into NOAA-OWP:master Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass catchment hydrofabric attributes to BMI models as model_params
2 participants