Finetune step_limiter!#1912
Merged
Merged
Conversation
Collaborator
Author
|
@evetion you might appreciate this first commit; it gets rid of many of the magic hardcoded numbers for threshold values. |
Collaborator
Author
evetion
reviewed
Oct 24, 2024
Member
There was a problem hiding this comment.
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.
evetion
approved these changes
Oct 24, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
UserDemandinflow and infiltration because of their importance in coupling.