-
Notifications
You must be signed in to change notification settings - Fork 234
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 support for Greybox model DOF counitng in degrees_of_freedom function #1512
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1512 +/- ##
==========================================
+ Coverage 76.99% 77.01% +0.02%
==========================================
Files 382 382
Lines 61994 62074 +80
Branches 10147 10165 +18
==========================================
+ Hits 47732 47809 +77
- Misses 11854 11858 +4
+ Partials 2408 2407 -1 ☔ View full report in Codecov by Sentry. |
idaes/core/util/model_statistics.py
Outdated
) + number_grey_box_equalities(block) | ||
|
||
|
||
def number_grey_box_equalities(block): |
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.
number_of_X
is less ambiguous in English.
Can add -> int
to indicate return type and, if it's possible, the expected type of block
.
In docs: "Method to" is superfluous and at any rate this is a function.
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.
done
idaes/core/util/model_statistics.py
Outdated
|
||
def number_grey_box_equalities(block): | ||
""" | ||
Method to return the number of equality constraints in GreyBox |
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 would say, "Compute total number of equality constraints for all GreyBox objects in this block."
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.
done
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 this, it is a case we have not considered and Greybox models are becoming more important. To start with, I had a few suggestions for more tests and modularization of the code.
However, a more significant request would be to add these tools and (and some test cases) into the diagnostics toolbox as well. We should at a minimum check that the DoF and model statistics output (number of variables and constraints) accurately reflect the model with a Greybox present. It might also be nice to add additional sub-rows in the model statistics for greybox components when they are present. Finally, we should look for any edge cases that might occur with the rest of the toolbox and if there are other things we need to consider when using greybox models.
idaes/core/util/model_statistics.py
Outdated
@@ -529,7 +555,7 @@ def deactivated_inequalities_generator(block): | |||
block : model to be studied | |||
|
|||
Returns: | |||
A generator which returns all indeactivated equality Constraint | |||
A generator which returns all in deactivated equality Constraint |
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.
The "in" should be removed here.
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.
done
idaes/core/util/model_statistics.py
Outdated
# Checks for greyboxes, and if they exist will add | ||
# input and output vars to var_set if they are free | ||
# inputs and outputs are defined names for greybox class and should always exist | ||
for grey_box in _iter_indexed_block_data_objects( |
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 think it would be good to have a number_of_greybox_variables
function to do this so that users can check that separately and so that we can test the code in isolation.
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.
added one for variables, and unfixed variables.
@@ -685,6 +689,42 @@ def test_degrees_of_freedom(m): | |||
assert degrees_of_freedom(m.b2) == -1 | |||
|
|||
|
|||
@pytest.mark.unit |
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.
We should also have test for the functions to collect the number of greybox variables and constraints.
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.
added tests
IIRC, outputs just get translated to equality constraints of the form |
@Robbybp : Right, so you can include equality constraints in a graybox and send them to the solver (ipopt) - but the variables in those constraints are not actually in pyomo space- and thus we can't count them (hence why here I am only looking at inputs and outputs and ignoring any internal equality constraints). (e.g. looking at this example) At this point, we just want to look at the graybox as a constraint that binds exposed pyomo inputs to outputs on it, and assume that the users properly setup the internal graybox model to have the correct DOFS... ideally we would have some metadata on the graybox that tells us its internal # of DOFs so we can include it in the total count. But I opt to avoid that for now... unless we want to add support for this now. |
idaes/core/util/model_statistics.py
Outdated
equalities = 0 | ||
for grey_box in _iter_indexed_block_data_objects( | ||
block, ctype=ExternalGreyBoxBlock, active=True, descend_into=True | ||
): | ||
equalities += len(grey_box.outputs) | ||
return equalities |
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.
As written, this will be incorrect for grey box models that have a nonzero number of "explicitly defined equality constraints" (as opposed to the equality constraints that are created to define the outputs). I think all that's needed to correct this is:
equalities += grey_box.get_external_model().n_equality_constraints()
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.
Right, did not think about this scenario! Updated to test graybox with suggestion, this now includes an example constraint that binds 2 inputs together.
def input_names(self):
return ["a1", "a2", "a3"]
def output_names(self):
return ["o1", "o2"]
def equality_constraint_names(self):
return ["a12"]
def evaluate_equality_constraints(self):
a1 = self._input_values[0]
a2 = self._input_values[1]
return [a1 * 0.5 - a2]
So I started adding display to diagnostics tools
|
idaes/core/util/model_statistics.py
Outdated
for in_var in grey_box.inputs: | ||
if not grey_box.inputs[in_var].fixed: | ||
var_set.add(grey_box.inputs[in_var]) | ||
for out_var in grey_box.outputs: | ||
if not grey_box.outputs[out_var].fixed: | ||
var_set.add(grey_box.outputs[out_var]) |
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.
Here, I think you need to make sure that these variables don't already appear in var_set
. If the ExternalGreyBoxBlock
is constructed with the inputs
or outputs
argument, then the block will contain references to the user-provided variables, which may have already been counted.
This is just waiting for @avdudchenko to extend the reporting functionality for diagnostics. This should be in the November release. |
Ready for another round of reviews, I updated model statistics and diagnostics to try and treat greybox models as part of full model,
|
consistency fix
I'm curious what happens in the structural analysis when you have a greybox model. I can't imagine the under/overconstrained subsystems are handled correctly. Do you know what happens in this case? |
Fixes
The current degrees_of_freedom function does not properly account for GreyBoxModel as a set of constraints, resulting in an incorrect number of DOFs being presented.
Summary/Motivation:
With increased usage of GreyBoxModel (specifically Reaktoro-pse) there is a need to support DOF counting when building IDAES model with GreyBoxModels
Changes proposed in this PR:
-add the capability to add unfixed input and output vars on GreyBox model to ComponentSet produced by unfixed_variables_in_activated_equalities_set
-add a counter for the number of "equalities" in gray box model in number_activated_equalities by adding number_grey_box_equalities method
Counting the number of equalities in GreyBox model:
GrayBox models do not provide traditional Pyomo Constraints that bind inputs to outputs as such, we need to assume how many GrayBox model DOFs there are.
In most cases (at least I can't think of any exceptions) GreyBoxes must be 0DOF in the eyes of Pyomo/IPOPT as each output must be some sort of function of provided inputs, regardless if the internal model in the GreyBox is 0DOF, an optimization, or it exposes internal constraints to solve the problem.
As such, it should be safe to assume that the number of outputs equals the number of equality constraints.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: