-
Notifications
You must be signed in to change notification settings - Fork 74
Pricetaker implementation for flexible desalination #1592
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
base: main
Are you sure you want to change the base?
Conversation
Note to self and @MarcusHolly - just a reminder that we'll need to add tests, in addition to review/suggesting changes (or implementing changes ourselves) |
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.
Thanks for all your work on this Radhakrishna! I've mainly left comments on the tutorial, but I can address most/all of them if you don't have the time to do so. As far as testing goes, it's prob easier to wait for the IDAES PR to be put up and merged.
For the file locations, I think we could either keep it as it is currently or just leave the .ipynb file in this tutorial folder and move the rest of the files to a place like watertap/flowsheets/flex_ro. I lean more towards the latter
@@ -0,0 +1,863 @@ | |||
{ |
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.
Would probably be helpful to have more text-based descriptions of what the following code will do so that users have an easier time going through and understanding this workflow on their own rather than relying solely on the in-line comments
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.
Would also probably be helpful to have an introductory blurb of what this tutorial will show and briefly introduce the important files (flowsheet.py, params.py, etc.)
"\n", | ||
"# Update the time-varying parameters other than the LMP, such as\n", | ||
"# demand costs and emissions intensity. LMP value is updated by default\n", | ||
"m.update_operation_params({\n", |
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.
Requires the pricetaker updates in IDAES to be merged: IDAES/idaes-pse#1633
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"fs.constrain_water_production(m)" |
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.
Inline-comment or description
design_blk : DesignModel | ||
Design model containing information on the peak capacity |
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 didn't notice the use of DesignModel anywhere in these files. Are we only using OperationModels? I assume this should be deleted.
"outputs": [], | ||
"source": [ | ||
"# Append LMP data to the model\n", | ||
"m.append_lmp_data(lmp_data=price_data[\"Energy Rate\"])\n", |
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 we just provide this directly when we instantiate Pricetaker? I vaguely remember a config option like this being setup on pricetaker, but I will go check.
Summary/Motivation:
This PR adds price-taker implementation for flexible desalination.
NOTE: This PR requires changes in this branch: https://github.com/radhakrishnatg/idaes-pse/tree/pt-improvements. So, until the above branch is merged with IDAES main, this PR should not be merged.
EDIT by @adam-a-a: we'll need to point to this IDAES PR: IDAES/idaes-pse#1633
EDIT by @lbianchi-lbl: this also depends on the changes in IDAES/idaes-pse#1579 (this has been merged)
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: