Conversation
This seems like something a user might want to control. 2 centimeter is probably a reasonable default, but why not make this part of the structure? FWIW, mf6 uses |
|
Yeah we may need to support that at some point, but not in this PR. We could either set it as a global setting in the TOML, or per node in the database. |
SouthEndMusic
left a comment
SouthEndMusic
left a comment
There was a problem hiding this comment.
Some comments, but overall looking good
core/src/solve.jl
Outdated
| q = clamp(q, min_flow_rate, max_flow_rate) | ||
|
|
||
| set_flow!(graph, inflow_edge, q, du) | ||
| _, src_level = get_level(p, inflow_id, t, du) |
There was a problem hiding this comment.
Looks like we don't ever use the first output of get_level, might be worth a refactor. I'm also not sure why you compare src_level to nothing, as the second output of get_level is always a number.
There was a problem hiding this comment.
Also isnothing looks a bit nicer to me
There was a problem hiding this comment.
Good call, I refactored get_level to return only a number, which I set to -Inf for Terminal to avoid reduction factors based on downstream level checks.
core/src/solve.jl
Outdated
| q *= reduction_factor(src_level - min_upstream_level, 0.02) | ||
| end | ||
|
|
||
| if dst_level !== nothing |
There was a problem hiding this comment.
Ah I also see that comparing against nothing this way was already in the code.
There was a problem hiding this comment.
We no longer need to check nothing with the new get_level.
| - $Q$ is the realized Outlet flow rate. | ||
| - $Q_\text{set}$ is the Outlet's target `flow_rate`. | ||
| - $Q_{\min}$ and $Q_{\max}$ are the Outlet `min_flow_rate` and `max_flow_rate`. | ||
| - $\phi$ is the reduction factor, which smoothly reduces flow based on all of these criteria: |
There was a problem hiding this comment.
I'd still be in favor of writing say
and then
There was a problem hiding this comment.
In principle the text is precise and with links to the reduction factor curious users can determine the exact equations. Though perhaps we can make the mathematical notation more complete besides the text.
| - $Q$ is the realized Pump flow rate. | ||
| - $Q_\text{set}$ is the Pump's target `flow_rate`. | ||
| - $Q_{\min}$ and $Q_{\max}$ are the Pump `min_flow_rate` and `max_flow_rate`. | ||
| - $\phi$ is the reduction factor, which smoothly reduces flow based on all of these criteria: |
3 participants
Fixes #1771, the remaining part after #1788 was merged. I split out #1789 as a separate issue, though in this PR a lot of the preparatory work is already done (all changes related to TabulatedRatingCurve).
This is breaking due to #1791 because of adding new optional columns.
It is also breaking that TabulatedRatingCurve and Pump now only support one outflow edge. As far as I know this wasn't used anywhere though. We cannot easily support that anymore since we now look at downstream levels. Users should instead use separate Pumps or TabulatedRatingCurves, which can share the same upstream Basin, but go to different downstream nodes.
I decided to start the reduction factors for reaching these limits from 2 centimeters away. There was one existing level based reduction factor, the level difference in the Outlet, which I also adjusted from 10 to 2 centimeter. I feel like 10 centimeter would be too much in areas where the level is controlled up to centimeter accuracies.