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

Expansion data file and model for expansion pumps #154

Open
wants to merge 10 commits into
base: v0.10-cleanup
Choose a base branch
from

Conversation

hskkanth
Copy link
Collaborator

Parses a .neinp file to read in expansion data. Made sure constraints for expansion pumps are added the same way as regular pumps

Parses a .neinp file to read in expansion data. Made sure constraints for expansion pumps are added the same way as regular pumps
@hskkanth hskkanth requested review from tasseff and rb004f March 23, 2023 19:03
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Attention: Patch coverage is 31.72043% with 254 lines in your changes are missing coverage. Please review.

Project coverage is 76.71%. Comparing base (5909b8c) to head (48cb3e2).

❗ Current head 48cb3e2 differs from pull request most recent head c12702f. Consider uploading reports for the commit c12702f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           v0.10-cleanup     #154      +/-   ##
=================================================
- Coverage          80.42%   76.71%   -3.71%     
=================================================
  Files                 42       43       +1     
  Lines               5501     5819     +318     
=================================================
+ Hits                4424     4464      +40     
- Misses              1077     1355     +278     
Files Coverage Δ
src/WaterModels.jl 100.00% <ø> (ø)
src/form/crd.jl 69.62% <ø> (ø)
src/io/common.jl 100.00% <100.00%> (ø)
src/prob/wf.jl 92.47% <85.71%> (-0.31%) ⬇️
src/core/objective.jl 86.66% <77.77%> (-0.84%) ⬇️
src/prob/ne.jl 76.19% <50.00%> (-10.48%) ⬇️
src/prob/owf.jl 83.87% <50.00%> (-16.13%) ⬇️
src/form/ncd.jl 84.98% <14.28%> (-1.53%) ⬇️
src/form/scip_constraints_variables.jl 91.17% <91.17%> (ø)
src/core/pump.jl 61.24% <20.00%> (-2.21%) ⬇️
... and 2 more

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5909b8c...c12702f. Read the comment docs.

@@ -102,6 +102,7 @@ include("util/obbt.jl")
# Deprecated functions.
include("deprecated.jl")

println("Running the cleaned up version of WaterModels.jl")
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

error_message *= "$(replace(comp_type, "_" => " ")) $(i) is not defined."
Memento.error(_LOGGER, error_message)
end
if(haskey(data,comp_type))
Copy link
Member

Choose a reason for hiding this comment

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

data should always include ne_pump as a key, even if there are no corresponding entries. This if statement should be removed.

warning_message *= "from node $(comp["node_fr"])."
Memento.warn(_LOGGER, warning_message)
end
if(haskey(data,comp_type))
Copy link
Member

Choose a reason for hiding this comment

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

Same here concerning the ne_pump key.

@@ -1139,6 +1151,78 @@ function _apply_pump_unit_transform!(
end
end

function _apply_ne_pump_unit_transform!(
Copy link
Member

Choose a reason for hiding this comment

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

Are there additional constants associated with network expansion, here, that need to be scaled? For example, construction cost?

src/core/pump.jl Outdated
base_flow = wm_data["per_unit"] ? wm_data["base_flow"] : 1.0
correction_func = x -> _correct_ne_pumps!(x, base_flow)
apply_wm!(correction_func, data; apply_to_subnetworks = true)
if(haskey(data,"ne_pump"))
Copy link
Member

Choose a reason for hiding this comment

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

Same comment that ne_pump should always be within data.

src/prob/wf.jl Outdated
@@ -114,6 +119,7 @@ function build_wf(wm::AbstractWaterModel)

# Add the objective.
objective_wf(wm)
println(wm.model)
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

src/prob/wf.jl Outdated
@@ -266,6 +272,7 @@ function build_mn_wf(wm::AbstractWaterModel)

# Add the objective.
objective_wf(wm)
println(wm.model)
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

@@ -12,9 +12,9 @@ dictionary of data). Here, `skip_correct` will skip data correction routines
(e.g., component status propagation) if set to `true`, and `per_unit` will
translate the data model to a per-unit measurement system if set to `true`.
"""
function parse_file(path::String; skip_correct::Bool = false, per_unit::Bool = true)
function parse_file(path::String; ne_path::String = "", skip_correct::Bool = false, per_unit::Bool = true)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be the preferred method for passing in network expansion data. For nonstandard network components, the preferred way would be to pass in modifications via an auxiliary JSON file, then update the base network data with these modifications. For example,

network = WaterModels.parse_file(network_path)
modifications = WaterModels.parse_file(modifications_path; skip_correct = true)
InfrastructureModels.update_data!(network, modifications)
WaterModels.correct_network_data!(network)

Here, network_path would be the original .inp file and modifications_path would be a JSON file that includes the data for ne_pump that would ultimately extend network. Does this make sense? Happy to help if there's a more concrete example of these data.

@@ -70,7 +70,9 @@ function correct_network_data!(data::Dict{String, <:Any}; per_unit::Bool = true)
correct_pipes!(data)
correct_des_pipes!(data)
correct_pumps!(data)
correct_ne_pumps!(data)
if(haskey(data,"ne_pump"))
Copy link
Member

Choose a reason for hiding this comment

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

Remove per comments above.

@@ -4,6 +4,13 @@ function _initialize_epanet_dictionary()
return data # Return an empty dictionary of the sectional data.
end

function _initialize_epanet_ne_dictionary(data::Dict{String,<:Any})
Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss whether these changes to epanet.jl are necessary or if these network expansion data can instead be passed in through an auxiliary JSON file.

To solve the problem for multiple non-consecutive representative days, tank volume needs to be updated at the end of every day.
Updating objective_ne to minimize the build cost over the first time step only

Adding temporary commits to support using scip
NC: Pump power lin apprx
wf: fixing binaries from a solution
fixing_binary_variables
update balerma.inp demand pattern
Currently, while merging ne tanks with regular tanks, if a non-numeric id is given to ne tanks, merging replaces one of the tanks. Fixed this by modifying the ids of ne tanks while merging
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.

2 participants