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

handle_invalid_soil_type #31

Merged
merged 8 commits into from
Sep 12, 2024
Merged

handle_invalid_soil_type #31

merged 8 commits into from
Sep 12, 2024

Conversation

ajkhattak
Copy link
Collaborator

The PR checks if the provided soil type(s) are not valid soil_types for the model such as waterbody, glacier etc. The model BMI Update returns input_precipitation as stream discharge output.

For now, invalid soil types should be appended to the end of the vG_soil_param.txt file, and set max_valid_soil_types (previously max_soil_types) to the number of valid soil types in the file. Later, we can make it more fancy.

Additions

  • None

Removals

  • None

Changes

  • Handles invalid soil types

Testing

  1. All existing tests passed

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)
  • [x ] Reviewers requested with the Reviewers tool ➡️

…etc., the model returns input precip as discharge.
Copy link
Contributor

@peterlafollette peterlafollette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I agree with the approach of having 12 valid soil types and then any soil after the 12th one is an invalid soil type, and also with having all precip becoming runoff for invalid soils.

Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few minor comments, mostly nits. There is one place that I would like to add a little safety, but it is a trivial change.

src/bmi_lgar.cxx Outdated Show resolved Hide resolved
src/bmi_lgar.cxx Outdated Show resolved Hide resolved
src/lgar.cxx Outdated Show resolved Hide resolved
configs/README.md Outdated Show resolved Hide resolved
state->lgar_mass_balance.volend_cm = state->lgar_mass_balance.volstart_cm;
state->lgar_mass_balance.volAET_cm = 0.0;
state->lgar_mass_balance.volrech_cm = 0.0;
state->lgar_mass_balance.volrunoff_cm += state->lgar_bmi_input_params->precipitation_mm_per_h * mm_to_cm;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is volrunoff and volQ the same thing? If not, this looks like a mass balance logical error since the precipitation is being accounted for twice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently they are the same, once we merge LGARTO, they will be different

src/bmi_lgar.cxx Outdated Show resolved Hide resolved
src/lgar.cxx Outdated Show resolved Hide resolved
src/lgar.cxx Show resolved Hide resolved
… an upper limit on num_soil_type as the default is already there.
@ajkhattak
Copy link
Collaborator Author

@hellkite500 I am going to revisit this volrunoff and volQ to make sure they are used properly. I know volQ is just an output variable that was added for parity with NWM variables, and not used anywhere in the code and in the mass_balance calculation, I will submit another PR if I found any inconsistencies.

@@ -15,7 +15,7 @@ A detailed description of the parameters for model configuration (i.e., initiali
| forcing_resolution | double (scalar)| - | sec/min/hr | temporal resolution | - | timestep of the forcing data. Recommended value of 3600 seconds. |
| endtime | double (scalar)| >0 | sec, min, hr, d | simulation duration | - | time at which model simulation ends |
| layer_soil_type | int (1D array) | - | - | state variable | - | layer soil type (read from the database file soil_params_file) |
| max_soil_types | int | >1 | - | - | - | maximum number of soil types read from the file soil_params_file (default is set to 15) |
| max_valid_soil_types | int | >1 | - | - | - | maximum number of valid soil types read from the file soil_params_file (default is set to 12; model not valid for soil_type = waterbody, lava, glacier, etc.) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted that if this value is more than the constant defined here

#define MAX_NUM_SOIL_TYPES 16

then an error/exit condition exists. I don't think this the number impacts the implementation details of this PR, but it is a somewhat odd out-of-band validation that is relevant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing if MAX_NUM_SOIL_TYPES is used anywhere, I am going to push a change to put this as an upper limit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that it isn't used in any of the BMI code paths, for sure, but in the src/main.cxx there is this check:

if(num_soil_types > (MAX_NUM_SOIL_TYPES -1))

@hellkite500 hellkite500 merged commit 46aa623 into master Sep 12, 2024
4 checks passed
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.

4 participants