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

pushing to upstream on new branch Methanol #23

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

pushing to upstream on new branch Methanol #23

wants to merge 12 commits into from

Conversation

parkyr
Copy link
Contributor

@parkyr parkyr commented May 8, 2024

Adding documentation try 1

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 first series of commits. Please address the comments I made and I will respond yours separately.

gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Show resolved Hide resolved
gdplib/methanol/methanol.py Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py 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 work, there are a few changed to be made and some duplicated constraints here and there

gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Outdated Show resolved Hide resolved
gdplib/methanol/methanol.py Show resolved Hide resolved
…re made during conflict res. please review for those in mind
@parkyr
Copy link
Contributor Author

parkyr commented May 31, 2024

Thanks for the quick review. Looks like some duplicates were created when I tried to resolve conflicts with the master branch and merged. Please review with this in mind. Thank you!

@parkyr
Copy link
Contributor Author

parkyr commented Jun 24, 2024

Issue #42 created:

the methanol results are infeasible in both master and methanol branch.

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 number of removed lines of code, besides the problem being now infeasible. Let's add the lines that were removed, and we can figure out the infeasibility later. I would suggest for the second one trying global LOA instead (it is very non-convex, so this might be the reason why the problem is infeasible). Another choice could be enumeration.

@@ -59,11 +59,6 @@ def fix_vars_with_equal_bounds(m, tol=1e-8):
v.name, lb, ub
)
)
raise InfeasibleError(
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a duplicate of above

@@ -485,18 +479,6 @@ def _total_flow(_m, _s):
self.build_stream_doesnt_exist_con(
m.single_stage_recycle_compressor_disjunct, 32
)
self.build_stream_doesnt_exist_con(
Copy link
Member

Choose a reason for hiding this comment

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

Why are these constraints removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate of above

@@ -545,31 +527,17 @@ def _total_flow(_m, _s):
* self.reactor_volume
* m.cheap_reactor.exists
)
e -= (
Copy link
Member

Choose a reason for hiding this comment

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

Why is this term removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate of above

e -= self.cheap_reactor_fixed_cost * m.cheap_reactor.exists
e -= (
self.expensive_reactor_variable_cost
* self.reactor_volume
* m.expensive_reactor.exists
)
e -= (
Copy link
Member

Choose a reason for hiding this comment

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

Why is this term removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is repeat from code below

e -= self.expensive_reactor_fixed_cost * m.expensive_reactor.exists
e -= (
(self.fix_electricity_cost + self.electricity_cost)
* m.single_stage_feed_compressor_disjunct.compressor_3.electricity_requirement
)
e -= (
Copy link
Member

Choose a reason for hiding this comment

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

Why is this term removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate of above

@@ -579,23 +547,11 @@ def _total_flow(_m, _s):
(self.fix_electricity_cost + self.electricity_cost)
* m.two_stage_feed_compressor_disjunct.compressor_6.electricity_requirement
)
e -= (
Copy link
Member

Choose a reason for hiding this comment

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

Why is this term removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate lines of above

@@ -609,18 +565,6 @@ def _total_flow(_m, _s):
self.cooling_cost
* m.two_stage_recycle_compressor_disjunct.cooler_18.heat_duty
)
e -= (
Copy link
Member

Choose a reason for hiding this comment

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

Why is this term removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate lines of above

@@ -348,7 +343,6 @@ def _total_flow(_m, _s):
m.cheap_feed_disjunct.feed_cons = c = pe.ConstraintList()
c.add(m.component_flows[1, 'H2'] == m.flow_1_composition['H2'] * m.flows[1])
c.add(m.component_flows[1, 'CO'] == m.flow_1_composition['CO'] * m.flows[1])
c.add(m.component_flows[1, 'CO'] == m.flow_1_composition['CO'] * m.flows[1])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the same line of code as above.

c.add(m.component_flows[1, 'CO'] == m.flow_1_composition['CO'] * m.flows[1])

@parkyr
Copy link
Contributor Author

parkyr commented Jun 25, 2024

There are a number of removed lines of code, besides the problem being now infeasible. Let's add the lines that were removed, and we can figure out the infeasibility later. I would suggest for the second one trying global LOA instead (it is very non-convex, so this might be the reason why the problem is infeasible). Another choice could be enumeration.

The lines removed were duplicate lines. I tried enumerate_solutions(), that also gave us infeasible soln.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants