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

Add water_network instance #59

Merged
merged 22 commits into from
Nov 4, 2024
Merged

Add water_network instance #59

merged 22 commits into from
Nov 4, 2024

Conversation

tristantc
Copy link
Contributor

This pull request incorporates the water_network instance.
Please review the changes and provide feedback. Thank you!

@ZedongPeng
Copy link
Member

Hi @tristantc . The linter check failed.

@tristantc
Copy link
Contributor Author

Thanks for the feedback, @ZedongPeng.
I will stick with ue for now, but I am willing to switch it if there is a better way to name it.

gdplib/README.md Outdated Show resolved Hide resolved
gdplib/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
cont,T
Copy link
Member

Choose a reason for hiding this comment

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

The CSV file looks small. Maybe instead of creating the CSV file, we should define a dictionary in the Python script. This also applies to TU.csv and feed.csv.

Copy link
Member

Choose a reason for hiding this comment

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

We would like to scale this instance to more cases, so I rather leave all in CSVs

gdplib/water_network/wnd.py Outdated Show resolved Hide resolved
gdplib/water_network/wnd.py Outdated Show resolved Hide resolved
gdplib/water_network/wnd.py Outdated Show resolved Hide resolved
gdplib/README.md Outdated Show resolved Hide resolved
Copy link
Member

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Good addition. Now, does it run with the benchmark code?

@@ -3,3 +3,4 @@
hda = "hda"
HDA = "HDA"
equil = "equil"
ue = "ue"
Copy link
Member

Choose a reason for hiding this comment

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

What is ue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ue stands for unit_exists


### Size

| Component | pwl | quadratic |
Copy link
Member

Choose a reason for hiding this comment

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

What about the original one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose not to upload the MINLP model because it's not a GDP model, but a GAMS model transformed into pyomo or are you asking about the model size table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I incorporated the original model without any approximation.

@@ -0,0 +1,5 @@
cont,T
Copy link
Member

Choose a reason for hiding this comment

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

We would like to scale this instance to more cases, so I rather leave all in CSVs

gdplib/water_network/wnd.py Outdated Show resolved Hide resolved
gdplib/water_network/wnd.py Outdated Show resolved Hide resolved
== ue.flow[mt, unit]
)

elif approximation == "piecewise":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

quadratic2 function fits original function better than quadratic, but not zero at zero flowrate.
Best known objective: $348,340
quadratic $346,672
quadratic2 $349,555

Copy link
Member

Choose a reason for hiding this comment

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

Either in the documentation or in the README explain what quadratic and quadratic2 are. I would recommend to rename the models to quadratic_zero_origin and quadratic_nonzero_origin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to hold onto both versions?

@tristantc
Copy link
Contributor Author

Good addition. Now, does it run with the benchmark code?

BigM and Hull MINLP reformulations work.
Neither the logic-based methods nor enumeration yield the optimal solution.

Copy link
Member

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

There are a few open comments to be addressed and then we can merge this

ue.streams,
doc="TU streams flowrate",
domain=pyo.NonNegativeReals,
# bounds=lambda _,i,k:(TU.loc[unit,'L'],100) # Upper bound for the flow rate from Ruiz and Grossmann (2009) is 100
Copy link
Member

Choose a reason for hiding this comment

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

Why not adding the same bound as the original instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The overall feed flow rate is less than 100.
It provides increased flexibility for multiple cases.

gdplib/water_network/wnd.py Show resolved Hide resolved
gdplib/water_network/wnd.py Show resolved Hide resolved
gdplib/water_network/wnd.py Show resolved Hide resolved
@ZedongPeng ZedongPeng self-requested a review October 15, 2024 16:39
@tristantc tristantc merged commit 9694230 into main Nov 4, 2024
2 checks passed
@tristantc tristantc deleted the water_network branch November 4, 2024 13:12
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.

3 participants