-
Notifications
You must be signed in to change notification settings - Fork 7
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 Reverse Electrodialysis model #78
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the REDstack.py file but I suppose that many of the comments for the other python file apply, so let's cover those for both files first, and then I will complete the review.
In general, there are a few typos, many "magic number" (numbers in the constraints that should be defined as parameters) and several comments that should be either removed or integrated as notes or documentation of the Pyomo model elements.
Another big one is that I want you to replace the Excel file with CSV files and sort out the imports
- Distribution of the high and low salinity streams. | ||
|
||
The objective function is: | ||
- Maximize the net present value (NPV) of the RED process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dollar units missing [$]
|
||
# Importing libraries | ||
import pyomo.environ as pyo | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be organized using a 'sort import' command
|
||
from pyomo.contrib.preprocessing.plugins import init_vars | ||
|
||
from pyomo.core.expr.logical_expr import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing things as * is not good practice. Have the methods explicitly imported
wnd_dir = os.path.dirname(os.path.realpath(__file__)) | ||
|
||
|
||
# The data.xlsx file contains the financial and stack parameters, and properties of the high and low salinity feed streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you import this from CSV files instead of Excel? These are harder to read in Unix.
True if the ports are the same, False otherwise | ||
""" | ||
x, y = val | ||
return re.findall(r'\d+', x) == re.findall(r'\d+', y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this regular expression line?
""" | ||
return m.TNP / sum(m._flow_into['rsu', sol] for sol in m.SOL) | ||
|
||
# New var. for pump capital cost: Z = sum(Q)**0.9, Z >= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is precisely what should go in the variable documentation
""" | ||
# [0.72–453.6] [m3 h-1]; CE = a + b(S)**n; S [0.2–126] [L s-1] Sinnot and Towler Single Stage Centrifugal | ||
return ( | ||
sum(6900 + 206 * m.pump_cap_cost_var[sol] for sol in m.SOL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These magic numbers should be parameters
return ( | ||
sum(m.stack_cost[ru] for ru in m.RU) # Cost of the stack and electrodes | ||
+ m.pump_cap_cost # Cost of the pumps | ||
+ m.TNP * 250 * m.eur2usd # Civil and infrastructure costs [USD kW-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 250?
""" | ||
return ( | ||
sum(m.operating_cost[ru] for ru in m.RU) + 0.02 * m.CAPEX | ||
) # O&M = 2%–4% CAPEX [USD y-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add parameter with documentation with complete reference
Pyomo.Expression | ||
Net Present Value [kUSD] as the difference between the benefits and the costs. | ||
""" | ||
# Assuming energy produced w selling price eq ep [USD] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why complete sentence as a note to the documentation
Introduce a new GDP model for the optimal design of the reverse electrodialysis process, including relevant data and documentation.