Flow demand with multiple priorities - #2500
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for FlowDemand nodes with multiple priority levels, extending the existing single-priority system to match the capabilities already available for UserDemand and LevelDemand nodes. The main changes enable FlowDemand nodes to specify demands across different priorities, improving the flexibility of the allocation algorithm.
- Refactored FlowDemand data structure to support multiple demand priorities
- Added a new test model demonstrating multi-priority flow demand functionality
- Updated documentation to reflect the expanded FlowDemand capabilities
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| python/ribasim_testmodels/ribasim_testmodels/allocation.py | Added new multi_priority_flow_demand_model and updated existing models for consistency |
| python/ribasim_testmodels/ribasim_testmodels/init.py | Exported the new multi_priority_flow_demand_model function |
| python/ribasim/ribasim/geometry/node.py | Updated terminology from "Main/Subnetwork" to "Primary/Secondary network" |
| docs/reference/node/flow-demand.qmd | Updated documentation to reflect multi-priority support |
| docs/reference/node/level-demand.qmd | Removed outdated single-priority limitation note |
| core/src/parameter.jl | Restructured FlowDemand to support priority matrices similar to other demand nodes |
| core/src/read.jl | Updated parsing logic to handle multi-priority FlowDemand data |
| core/src/allocation_*.jl | Modified allocation algorithms to work with the new multi-priority structure |
| core/test/*.jl | Updated tests to work with the new FlowDemand structure |
| basin_data = [basin.Profile(area=1e4, level=[0.0, 10.0]), basin.State(level=[1.0])] | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider moving the basin_data definition closer to where it's used or making it a function parameter to improve code organization and reduce the distance between definition and usage.
| basin_data = [basin.Profile(area=1e4, level=[0.0, 10.0]), basin.State(level=[1.0])] |
| zeros(Bool, length(node_id), length(demand_priorities)) | ||
| demand_itp::Vector{Vector{ScalarLinearInterpolation}} = | ||
| trivial_linear_itp_fill(demand_priorities, node_id; val = NaN) | ||
| demand::Matrix{Float64} = fill(NaN, length(node_id), length(demand_priorities)) |
There was a problem hiding this comment.
Using NaN as a default value for demand could lead to unexpected behavior if not properly handled. Consider using a more explicit sentinel value or documenting the NaN usage pattern clearly.
| demand::Matrix{Float64} = fill(NaN, length(node_id), length(demand_priorities)) | |
| demand::Matrix{Union{Float64, Missing}} = fill(missing, length(node_id), length(demand_priorities)) |
| d = demand_function(node_id) | ||
| JuMP.set_normalized_coefficient(constraint_lower, rel_error_lower, d) | ||
| JuMP.set_normalized_coefficient(constraint_lower, target_demand_fraction, -d) | ||
|
|
||
| if deactivate_when_no_demand && iszero(d) | ||
| if isnan(d) | ||
| # d == NaN means there is no demand, so set the rhs |
There was a problem hiding this comment.
[nitpick] The NaN check for demand could be replaced with a more explicit approach using a separate boolean flag to indicate whether demand exists, which would be more readable and less error-prone.
| level_demand.target_storage_min[basin_id] = fill(NaN, n_demand_priorities) | ||
| level_demand.target_storage_max[basin_id] = fill(NaN, n_demand_priorities) | ||
| level_demand.storage_prev[basin_id] = 0.0 | ||
| level_demand.storage_allocated[basin_id] = zeros(n_demand_priorities) | ||
| level_demand.storage_demand[basin_id] = zeros(n_demand_priorities) | ||
| level_demand.storage_demand[basin_id] = fill(NaN, n_demand_priorities) |
There was a problem hiding this comment.
Initializing with NaN may cause issues if these values are used in calculations before being properly set. Consider using a more appropriate default value or ensuring all NaN values are replaced before use.
| # of the constraint to a large negative value which effectively deactivates the constraint | ||
| JuMP.set_normalized_rhs(constraint_lower, -1e10) | ||
| else | ||
| JuMP.set_normalized_coefficient(constraint_lower, rel_error_lower, max(1e-6, d)) | ||
| JuMP.set_normalized_coefficient(constraint_lower, target_demand_fraction, -d) | ||
| JuMP.set_normalized_rhs(constraint_lower, 0) |
There was a problem hiding this comment.
It seems to me that having constraint of the form x < big_number can cost performance relative to removing this constraint?
There was a problem hiding this comment.
is something like this possible:
JuMP.set_normalized_coefficient(constraint_lower, rel_error_lower, 0)
JuMP.set_normalized_coefficient(constraint_lower, target_demand_fraction, 0)
JuMP.set_normalized_rhs(constraint_lower, 0)
There was a problem hiding this comment.
I'm fiddling with the constraints for the error on the flow demand flow here, because I have one constraint for all priorities. That will be better in #2522, where I have separate flow variables, corresponding errors and constraints per priority for which there is a demand
| demand_itps = flow_demand.demand_itp[flow_demand_id.idx] | ||
| for (demand_priority_idx, demand_itp) in enumerate(demand_itps) | ||
| if flow_demand.has_demand_priority[ | ||
| flow_demand_id.idx, | ||
| demand_priority_idx, | ||
| ] | ||
| has_any_demand_priority = true |
There was a problem hiding this comment.
naming: demand_itps and demand_itp? Is that demand_interpolations?
Fixes #2320.