Update pricetaker tutorial - #1822
Conversation
| results = solver.solve(m, tee=True) | ||
|
|
||
| # This is line that is causing the failure on Linux. But I don't know why. | ||
| # results = solver.solve(m) |
There was a problem hiding this comment.
This line is causing failure on Linux, but is fine on Windows. Unsure how to resolve the problem as the error output log gives almost no information
There was a problem hiding this comment.
If there is a failure to converge to a solution, then it is usually because of an issue with initialization or scaling. It could also potentially have to do with problems in the formulation, but since you say the solve passes on Windows, I'd assume the former.
There was a problem hiding this comment.
Did some testing with Kurby- It's a problem with the watertap solver, which just rejects the problem because it includes binary variables. Going to change to the IDAES solver.
The point of this "solve" is not to generate a real solution, but to allow the post_solve functions to be tested.
jakechurchi
left a comment
There was a problem hiding this comment.
Commenting to guide changes + make future reviewing easier
| assert hasattr(m, "posttreatment_unit_commitment_shutdown") | ||
|
|
||
| # Limit hours when start-ups and shutdowns can occur to reflect when operators are on-site | ||
| fs.add_working_hours_constraint(m) |
There was a problem hiding this comment.
Difficult to do assertions for this function because it only exists for a subset of the hours
| def test_add_expressions(self, system_frame): | ||
| m, price_data, peak_hours = system_frame | ||
|
|
||
| fs.add_useful_expressions(m) |
There was a problem hiding this comment.
I feel like this function should either be scrapped, or some of the functions below should be included.
There was a problem hiding this comment.
Is there a reason to piece the build out like this rather than just having a single build function that does all the .add_{stuff} and returns the fully constructed model?
There was a problem hiding this comment.
For testing purposes this division it helps identify where issues would arise. Also, some of the add functions are optional (ex. add_working_hours_constraint) so I wouldn't want that in the build function.
There was a problem hiding this comment.
The ones that could be included in the build would probably be add_demand_and_fixed_costs, add_useful_expressions, and constrain_water_production.
| @pytest.mark.unit | ||
| def test_fixing_operations(self, system_frame): | ||
| m, price_data, peak_hours = system_frame | ||
| fs.fix_operations_for_first_four_days(m, peak_hours=peak_hours) |
There was a problem hiding this comment.
Is it worth adding assertions for this? Feel like I'm missing the point of those assert statements. If there is no error on calling the function, that means those expressions/vars/constraints were successfully created, right?
There was a problem hiding this comment.
This is a pretty rough diagram, but I want to establish the treatment steps, the parallel ro trains, and the costed flows.
|
|
||
|
|
||
| def calculate_replacement_costs(m): | ||
| """Adds expression for the replacement cost""" |
There was a problem hiding this comment.
New, def needs documentation
| ) | ||
|
|
||
|
|
||
| def calculate_flexibility_metrics( |
There was a problem hiding this comment.
New, needs to be included in the documentation
| m.LVOF = Var(initialize=LVOF) | ||
|
|
||
|
|
||
| def add_useful_expressions(m): |
There was a problem hiding this comment.
Highly consider expanding or deleting this
| ) | ||
|
|
||
|
|
||
| def fix_operations_for_first_four_days(m, peak_hours=None): |
There was a problem hiding this comment.
Also not used in tutorial example? Review the comments on this function
| ) # Plant must be on | ||
|
|
||
|
|
||
| ### NOT USED IN THE TUTORIAL EXAMPLE - That might mean they aren't being tested? ### |
There was a problem hiding this comment.
Revisit these functions after drafting the documentation and having some working tests
| @@ -21,7 +21,7 @@ | |||
| from watertap.flowsheets.flex_desal import wrd_ro_flowsheet as fs | |||
| from watertap.flowsheets.flex_desal import utils | |||
| from watertap.flowsheets.flex_desal.params import FlexDesalParams | |||
| from watertap.core.solvers import get_solver | |||
| from idaes.core.solvers import get_solver | |||
There was a problem hiding this comment.
Important to note WATERTAP get_solver will fail on Mac and Linux!
There was a problem hiding this comment.
these notebooks aren't being tested yet but this will be an issue once they are
kurbansitterley
left a comment
There was a problem hiding this comment.
This is a totally new tutorial and flowsheet rather than an update to the existing tutorial, correct?
Because this PR would add two flex desal flowsheets and tutorials in the repository, before I go much further I'd like to see some changes to organization.
- the two flowsheets/tutorials should be in their own directories within
*/flex_rodirectories with their own assets/utility files - within their own directories any data/jsons/pngs should be in an assets folder or something (I know that other tutorials don't follow this convention but I think they should in a future mod)
- in general I am not wild about
wrd_being prepended to all the WRD-inspired additions. It is not really descriptive to what the files are and will be lost on a new user approaching WaterTAP tutorials. I don't have any good suggestions atm, but - what is the primary difference between the two flowsheets? - our other tutorials have more descriptive names (sorta the same point as the last bullet) so I'd like to see
mp_modelandwrd_modelchanged at minimum
I know we are sorta on a time crunch so interested in what @adam-a-a thinks here.
This is a good point. I would say the biggest difference is that energy intensity is a function of flowrate in the WRD case. So naming convention could be "flex recovery" (for original tutorial) and "flex flow" for the WRD one? Will wait for feedback before renaming everything though. |
| # NOTE: This function is needed until Pyomo's Gurobi writer can handle | ||
| # general nonlinear constraints. Once Pyomo supports this feature, this | ||
| # function can be removed from the codebase. Since this function is | ||
| # needed temporarily, tests are not added for this function. |
There was a problem hiding this comment.
If my understanding is correct, this test can be deleted because gurobi can deal with general nonlinear constraints?
| solver.set_instance(m) | ||
| return solver | ||
|
|
||
| if m.params.surrogate_type == "quadratic_surrogate": |
There was a problem hiding this comment.
This would need to be like m.params.ro.surrogate_type
Fixes/Resolves:
Addresses issue #1806
Summary/Motivation:
The current pricetaker tutorial was focused on one facility. This PR provides a more complicated example.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: