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.

Sign up for GitHub

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

Jump to bottom

adjusted_BMI_set_get_functions #127

Merged
merged 6 commits into from
Aug 30, 2024
Merged

adjusted_BMI_set_get_functions #127

merged 6 commits into from
Aug 30, 2024

Conversation

ajkhattak
Copy link
Contributor

@ajkhattak ajkhattak commented Aug 28, 2024 •
edited
Loading

The PR fixes issue #93 - make the Set_value and Set_value_at_indices functions consistent with the BMI 2.0 standard. No Changes to the functionality of the model. closes #93

Additions

  • Modified Set_value and Set_value_at_indices functions

Removals

  • None

Changes

  • None

Testing

  1. All existing tests passed, and results are unchanged
  2. Manually tested calibratable parameters using ngen realization.json to ensure they are working properly

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

hellkite500
hellkite500 reviewed Aug 30, 2024
View reviewed changes
src/bmi_cfe.c Outdated
@@ -1787,6 +1786,10 @@ static int Get_var_nbytes (Bmi *self, const char *name, int * nbytes)
for (i = 0; i < INPUT_VAR_NAME_COUNT; i++) {
if (strcmp(name, input_var_names[i]) == 0) {
item_count = input_var_item_count[i];
if (strcmp(name, "soil_moisture_profile") == 0 || strcmp(name, "soil_layer_depths_m") == 0) {
cfe_state_struct *cfe_ptr;
item_count = ((cfe_state_struct *)(self->data))->soil_reservoir.n_soil_layers + 1;
Copy link
Member

Choose a reason for hiding this comment

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

why is this +1?

Copy link
Contributor Author

@ajkhattak ajkhattak Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this +1 is actually a bug. I followed the concept from the initialization stage (here), what looks to me is that they tried to start the index at 1 (see), but the BMI Set_value does not work this way, so we need to fix it.

src/bmi_cfe.c Outdated Show resolved Hide resolved
src/bmi_cfe.c Outdated Show resolved Hide resolved
ajkhattak and others added 3 commits August 30, 2024 10:46
@ajkhattak @hellkite500
Update src/bmi_cfe.c
3f43749 
Co-authored-by: Nels <nfrazier@lynker.com>
@ajkhattak
fixed bug related to +1 in the AET rootzone memory allocation, remove…
e59aeb8 
…d unused variables num_cells and soil_layer_depths.
@ajkhattak
fixed default n_soil_layers.
3aaf7d6
@ajkhattak ajkhattak merged commit 0b488a1 into master Aug 30, 2024
4 checks passed
@ajkhattak ajkhattak mentioned this pull request Aug 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellkite500 hellkite500 hellkite500 approved these changes

@aaraney aaraney Awaiting requested review from aaraney

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adjust implementations of set_value and set_value_at_indices to more closely follow BMI standard
2 participants
@ajkhattak @hellkite500