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

Implement standard definitions of Set_value & Set_value_at_indices #104

Closed

Conversation

madMatchstick
Copy link
Contributor

@madMatchstick madMatchstick commented Feb 7, 2024

Adjusts two BMI functions,

  1. Set_value()
  2. Set_value_at_indices()

to "standard" BMI definitions according to examples and readme documents

  1. Also - minor is_aet_rootzone (boolean flag) name updates left over from PR bug_fix_aet rootzone #105

Changes

  • bmi_c.c

Testing

  1. NextGen Integration Workflow in parent repo passes; this checks ngen build/run coupling cfe & pet
  2. Standalone and BMI unit testing workflow (yml here)
  3. NextGen Realization in SoilFreezeThaw; coupling cfe, sft, smp & pet
  4. Config sets both is_sft_coupled & is_aet_rootzone to true. e.g.,
is_sft_coupled=true
ice_content_threshold=0.15
is_aet_rootzone=true
soil_layer_depths=0.1,0.4,1.0,2.0
max_rootzone_layer=2

as outlined in configs/readme here & here.

Notes

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
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

@madMatchstick madMatchstick changed the title Implement standard definitions of Set_values & Set_value_at_indices Implement standard definitions of Set_value & Set_value_at_indices Feb 7, 2024
@madMatchstick madMatchstick marked this pull request as ready for review February 12, 2024 19:28
@ajkhattak ajkhattak mentioned this pull request Feb 15, 2024
7 tasks
// Use nested call to "by index" version

// Here, for now at least, we know all the variables are scalar, so
int inds[] = {0};

// Then we can just ...
return Set_value_at_indices(self, name, inds, 1, array);
return Set_value_at_indices(self, name, inds, 1, array);*/
Copy link
Contributor

Choose a reason for hiding this comment

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

this only works for doubles and not for array of doubles, so it will potentially break the coupling with soil_moisture_profile which is used in rootzone-based AET.

@ajkhattak
Copy link
Contributor

@madMatchstick while reviewing and testing this PR, which breaks the coupling with SMP, I found a bug in the rootzone-based AET (not directly related to this PR). so I think that PR (104) should be merged before this one, as we need to ensure that Set_value and Set_valued_at_indices work properly for rootzone-based AET.

@madMatchstick
Copy link
Contributor Author

@madMatchstick while reviewing and testing this PR, which breaks the coupling with SMP, I found a bug in the rootzone-based AET (not directly related to this PR). so I think that PR (104) should be merged before this one, as we need to ensure that Set_value and Set_valued_at_indices work properly for rootzone-based AET.

Ok, thanks Ahmad - looking at the PR you linked, I now understand what you mean by "breaks when coupling with SMP" as the aet_rootzone flag was always OFF. I'll merge that one first and go from there....

@ajkhattak
Copy link
Contributor

sounds good.. we have to make sure that get_var_nbytes returns the correct nbytes for the soil_moisture_profile.

@madMatchstick madMatchstick marked this pull request as draft July 15, 2024 19:07
@ajkhattak
Copy link
Contributor

@madMatchstick should we close this??

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

Successfully merging this pull request may close these issues.

2 participants