-
Notifications
You must be signed in to change notification settings - Fork 5
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
Finetune step_limiter!
#1912
Finetune step_limiter!
#1912
Conversation
@evetion you might appreciate this first commit; it gets rid of many of the magic hardcoded numbers for threshold values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments:
For the next time, please refactor in separate PR's, mixing it makes the PR and the git history in the future hard to read. I do like the refactor, tackling the magic numbers and the basin complexity 👍🏻.
Could you provide a comment in the PR description on what you introduce and why? I'll give more detailed comments in the code below.
After reading it completely, my main confusion is that we clamp the minimum flow out of a basin and not the maximum flow? I thought the reduction factors trigger so a (maximum) flow cannot empty a basin.
Follow-up of #1911.
Fixes #1897 (?).
Fixes #1838 (how could we test that negative flows where they should not be no longer occur? Maybe add a callback that explicitly checks for non-decreasing states)
I focus in this PR on
UserDemand
inflow and infiltration because of their importance in coupling.