Add Storage and Surrogate Support to the Price-taker class #1633
Conversation
| if self.config.fixed_design_data is not None: | ||
| # The design is fixed, so all the desired quantities are defined as parameters | ||
| for param_name, param_value in self.config.fixed_design_data.items(): | ||
| setattr(self, param_name, Param(initialize=param_value, mutable=True)) |
There was a problem hiding this comment.
Any reason why you chose mutable Params over fixing Vars ?
There was a problem hiding this comment.
Not quite sure which option is preferable. If we define a Var and fix it, it would allow the user to unfix it. If they do that, then the design variable would be unconstrained and it may lead to an unexpected behavior.
I feel that declaring it as a Param would prevent unfixing and it ensures that the design is always fixed. I can change it to fixing Var if that is the convention we are following everywhere.
…to pt-improvements
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1633 +/- ##
==========================================
- Coverage 77.46% 77.36% -0.10%
==========================================
Files 394 394
Lines 64746 64979 +233
Branches 10896 10947 +51
==========================================
+ Hits 50156 50273 +117
- Misses 12081 12197 +116
Partials 2509 2509 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @declare_process_block_class("StorageModel") | ||
| class StorageModelData(ProcessBlockData): |
There was a problem hiding this comment.
Is there any reason why we don't just have this in a separate file?
There was a problem hiding this comment.
It is a short piece of code, so I just retained it in the same file. Anyways, users will be importing models from idaes.apps.grid_integration, so I thought it doesn't matter if we put it in this file or in a separate file.
| def _add_upper_bound(var_name, bound): | ||
| if isinstance(bound, (Var, Expression)): | ||
| setattr( | ||
| self, | ||
| var_name + "ub_con", | ||
| Constraint( | ||
| expr=getattr(self, var_name) <= bound, | ||
| doc=f"Constrains the maximum value of {var_name}", | ||
| ), | ||
| ) | ||
|
|
||
| else: | ||
| getattr(self, var_name).setub(bound) | ||
|
|
||
| _add_upper_bound("charge_rate", self.config.max_charge_rate) | ||
| _add_upper_bound("discharge_rate", self.config.max_discharge_rate) | ||
| _add_upper_bound("final_holdup", self.config.max_holdup) | ||
| _add_upper_bound("initial_holdup", self.config.max_holdup) |
There was a problem hiding this comment.
If I understand correctly, this _add_upper_bound function essentially provides the upper bound for a variable by adding a constraint (named m.charge_rate_ub_con in the case of charge_rate, for example) when a Var or Expression is provided.
However, when a float, int, etc., is used, this constraint would not be constructed and the upper bound of the Var would just be set directly.
While it seems like a trivial and subtle difference, it also seems less than ideal to allow variation in model structure like this (where one version includes upper bound constraints and another analogous version does not, simply based on input provided).
Is there a reason why we don't just (1) check whether instance is a Var or Expression, and if so, just do setub(pyo.value(bound)) ? The only potential issue that I can foresee with that is if an indexed Var or Expression is passed in, but haven't checked. Are there other reasons?
There was a problem hiding this comment.
The Var and Expression are needed for the case where the design is a decision variable. Since the value of the variable is unknown, setub(pyo.value(bound)) will not work.
I agree that having different behavior for int/float vs. Var/Expression is not ideal. But, we cannot avoid it. To keep it consistent, we can implement all them as constraints regardless of the data type. But, that may impact the performance of some solvers. So, for now, I thought we can leave it as it is.
|
@radhakrishnatg, @adam-a-a: Moving this to the Nov release. |
|
@radhakrishnatg, @adam-a-a: Are you able now to look into this? |
|
@ksbeattie the main motivation behind this PR is related to incorporating a pricetaker model in WaterTAP. However, when pointing to this PR from WaterTAP, the current version of IDAES breaks tests on TAP. we could still bring this pr in anyhow, but a bigger issue would be getting watertap to work with latest idaes |
|
@radhakrishnatg should now be able to revisit this. |
|
@adam-a-a would like it if this could stay in the Nov 2025 release. |
MarcusHolly
left a comment
There was a problem hiding this comment.
Minor comments - otherwise LGTM
| def _build_variable_design_model(self): | ||
| data = self.config.variable_design_data |
| CONFIG.declare( | ||
| "capacity", | ||
| ConfigValue(doc="Maximum capacity of the commodity"), | ||
| ) |
There was a problem hiding this comment.
Specify domain for this?
There was a problem hiding this comment.
The input can be anything: Var, Expression, int, or float. So, I did not specify the domain.
|
@ksbeattie @sufikaur we're unsure why codecov seemingly isn't registering tests intended to address uncovered lines and increase coverage |
adam-a-a
left a comment
There was a problem hiding this comment.
LGTM - any remaining issues can be logged in an issue.
Summary/Motivation:
This PR adds new features to the price-taker class. This is WIP. This PR needs to be merged after merging #1579
Changes proposed in this PR:
pd.DataFrameanddict, respectively.StorageModelis found in the flowsheet instance, then linking constraints are now added automatically.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: