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

Compute mean realized flows #1514

Merged
merged 9 commits into from
Jun 6, 2024
Merged

Compute mean realized flows #1514

merged 9 commits into from
Jun 6, 2024

Conversation

SouthEndMusic
Copy link
Collaborator

Fixes #1356.

@SouthEndMusic
Copy link
Collaborator Author

Last to do: Add test for mean realized user demand

@SouthEndMusic SouthEndMusic changed the title Compute mean realized flows, fix/add tests Compute mean realized flows Jun 3, 2024
@SouthEndMusic SouthEndMusic requested review from verseve, JoostBuitink and visr and removed request for verseve and JoostBuitink June 3, 2024 11:14
core/src/read.jl Outdated
Comment on lines 1056 to 1073
mean_realized_flows = Dict{Tuple{NodeID, NodeID}, Base.RefValue{Float64}}()

# Find edges that realize a demand
for edge_metadata in values(graph.edge_data)
(; type, edge) = edge_metadata

src_id, dst_id = edge
user_demand_inflow = (type == EdgeType.flow) && (dst_id.type == NodeType.UserDemand)
level_demand_inflow =
(type == EdgeType.control) && (src_id.type == NodeType.LevelDemand)
flow_demand_inflow =
(type == EdgeType.flow) && has_external_demand(graph, dst_id, :flow_demand)[1]

if user_demand_inflow || flow_demand_inflow
mean_realized_flows[edge] = Ref(0.0)
elseif level_demand_inflow
mean_realized_flows[(dst_id, dst_id)] = Ref(0.0)
end
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you went for Ref here instead of a Float? Is it for the ergonomics of being able to do

value = dict[key]
value[] += x
value[] /= dt
value = dict[key]
value += x
value /= dt
dict[key] = value

i.e. having to put it back in? I worry a bit about the performance of this approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got rid of the use of RefValue{Float64}, but now there are more dictionary lookups, e.g.

for key in keys(mean_input_flows)
mean_input_flows[key] = mean_input_flows[key] / Δt_allocation
end
# Divide by the allocation Δt to obtain the mean realized flows
# from the integrated flows
for (edge, value) in mean_realized_flows
if edge[1] == edge[2]
# Compute the mean realized demand for basins as Δstorage/Δt_allocation
_, basin_idx = id_index(basin.node_id, edge[1])
mean_realized_flows[edge] = value + u[basin_idx]
end
mean_realized_flows[edge] = mean_realized_flows[edge] / Δt_allocation
end

Is there a way to circumvent this?

Copy link
Member

Choose a reason for hiding this comment

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

Dict lookups are quite fast, so I doubt it's a serious issue. I haven't looked into it, but perhaps the compiler can figure out it can optimize this with a single lookup. Also I just tried this, and surprisingly it works. Could be of course that there are still two lookups behind this syntax:

julia> dict = Dict{Int, Int}(1 => 1)
Dict{Int64, Int64} with 1 entry:
  1 => 1

julia> dict[1] += 1
2

julia> dict
Dict{Int64, Int64} with 1 entry:
  1 => 2

@SouthEndMusic SouthEndMusic requested a review from visr June 5, 2024 08:30
@visr visr merged commit c3d5a40 into main Jun 6, 2024
25 checks passed
@visr visr deleted the mean_realized_demands branch June 6, 2024 08:26
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.

Use average flows over allocation intervals for realized in allocation.arrow
2 participants