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

removed N_nash_subsruface bmi parameter #131

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

ajkhattak
Copy link
Contributor

The PR removed the bmi parameter N_nash_subsurface from the list of calibratable parameters as we don't calibrate it.

Additions

  • None

Removals

  • remove N_nash_subsurface

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

@ajkhattak ajkhattak requested a review from aaraney September 5, 2024 16:57
configs/README.md Outdated Show resolved Hide resolved
configs/README.md Outdated Show resolved Hide resolved
configs/README.md Outdated Show resolved Hide resolved
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 suggestions for denoting if a parameter is calibratable or only applicable when surface_runoff_scheme is NASH_CASCADE. Purely stylistic and subjective; Certainly not required.

The code modifications look solid.

@SnowHydrology
Copy link
Contributor

@aaraney Did you build and run CFE with the changes?

@aaraney
Copy link
Member

aaraney commented Sep 5, 2024

@SnowHydrology, I did not. I trusted that the GH actions would have failed if there was an issue.

@madMatchstick
Copy link
Contributor

@SnowHydrology, I did not. I trusted that the GH actions would have failed if there was an issue.

Also - note that surface_runoff_scheme=NASH_CASCADE is tested for CFE v2.0, here. Looks good to me.

Copy link
Contributor

@SnowHydrology SnowHydrology left a comment

Choose a reason for hiding this comment

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

After README cleanup, this can be merged

configs/README.md Outdated Show resolved Hide resolved
@ajkhattak
Copy link
Contributor Author

thanks all for the review, I have merged/incorporated @aaraney suggestions

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.

Looks great! Thanks for making the changes, @ajkhattak! 🎉

@aaraney aaraney dismissed SnowHydrology’s stale review September 6, 2024 15:34

No longer applicable

@aaraney aaraney merged commit 29d3f98 into master Sep 6, 2024
4 checks passed
@aaraney aaraney deleted the ajk/Nash_subsurface_calib_param branch September 6, 2024 15:34
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.

4 participants