Implicit constraint#3870
Conversation
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
Thanks for the PR! At first glance, this looks pretty good. It will take a little while to get through this.
- Can you post a simple example of calling an Incidence Analysis method on a model with a gray box block
- I probably would have tried to do this only with something like your
EGBConstraintBodyobject, i.e., I would not have createdExternalGreyBoxConstraint. Did you consider this? What was the motivation to add an "official"Componentobject?
|
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
Some initial comments. I haven't had time to think through the implications of ExternalGreyBoxConstraint yet.
This is reminding me that our grey-box infrastructure could use some better documentation.
| external_grey_box_model, | ||
| inputs=None, | ||
| outputs=None, | ||
| build_implicit_constraint_objects=False, |
There was a problem hiding this comment.
Any reason this is optional?
There was a problem hiding this comment.
Only to ensure there is backward compatibility. I decided to make sure there was still a way to guarantee the model would behave the same as before.
| for input_name in ext_model.input_names(): | ||
| incident_variables.append(self._parent_model.inputs[input_name]) |
There was a problem hiding this comment.
We can't assume that inputs are indexed by input_names(). If inputs are provided to set_external_model, they will be a reference-to-list of VarData. Here's an example that fails with the current implementation:
inputs-as-reference.py
import numpy as np
import pyomo.environ as pyo
from scipy.sparse import coo_matrix
from pyomo.contrib.pynumero.interfaces.external_grey_box import (
ExternalGreyBoxBlock,
ExternalGreyBoxModel,
)
from pyomo.contrib.incidence_analysis import IncidenceGraphInterface
class MyGreyBox(ExternalGreyBoxModel):
def n_inputs(self):
return 4
def input_names(self):
return [f"x[{i}]" for i in range(1, self.n_inputs() + 1)]
def n_equality_constraints(self):
return 3
def equality_constraint_names(self):
return [f"eq[{i}]" for i in range(1, self.n_equality_constraints() + 1)]
def set_input_values(self, input_values):
if len(input_values) != self.n_inputs():
raise ValueError(
f"Expected {self.n_inputs()} inputs, got {len(input_values)}."
)
self._inputs = np.asarray(input_values, dtype=float)
def evaluate_equality_constraints(self):
x1, x2, x3, x4 = self._inputs
return np.array(
[
x2 * x3**1.5 * x4 - 5.0,
x1 - x2 + x4 - 4.0,
x1 * np.exp(-x3) - 1.0,
],
dtype=float,
)
def evaluate_jacobian_equality_constraints(self):
x1, x2, x3, x4 = self._inputs
# Rows correspond to eq[1], eq[2], eq[3]
# Cols correspond to x[1], x[2], x[3], x[4]
rows = np.array([0, 0, 0, 1, 1, 1, 2, 2], dtype=int)
cols = np.array([1, 2, 3, 0, 1, 3, 0, 2], dtype=int)
data = np.array(
[
x3**1.5 * x4, # d(eq1)/d(x2)
1.5 * x2 * x3**0.5 * x4, # d(eq1)/d(x3)
x2 * x3**1.5, # d(eq1)/d(x4)
1.0, # d(eq2)/d(x1)
-1.0, # d(eq2)/d(x2)
1.0, # d(eq2)/d(x4)
np.exp(-x3), # d(eq3)/d(x1)
-x1 * np.exp(-x3), # d(eq3)/d(x3)
],
dtype=float,
)
return coo_matrix(
(data, (rows, cols)),
shape=(self.n_equality_constraints(), self.n_inputs()),
)
m = pyo.ConcreteModel()
m.x = pyo.Var([1, 2, 3, 4], bounds=(0.0, None), initialize=1.0)
m.objective = pyo.Objective(
expr=m.x[1] ** 2 + 2 * m.x[2] ** 2 + 3 * m.x[3] ** 2 + 4 * m.x[4] ** 2
)
m.grey_box = ExternalGreyBoxBlock()
m.grey_box.set_external_model(
MyGreyBox(),
inputs=[m.x[i] for i in range(1, 5)],
build_implicit_constraint_objects=True,
)
igraph = IncidenceGraphInterface(m)
matching = igraph.maximum_matching()There was a problem hiding this comment.
I was not even aware you could do that (hence your comment about documentation I guess). What is the canonical way to defining inputs and names then?
There was a problem hiding this comment.
I'm not sure if there is a "canonical" way. Here's what I remember:
- Carl's original implementation created new variables on the
ExternalGreyBoxBlock. These variables are indexed by the names defined on theExternalGreyBoxModel. The names are also used under the hood inPyomoNLPWithGreyBoxBlocks(or somewhere) as keys (or something). - I did not want to duplicate variable for every input, so I added the option to provide input (and output) variables if they already exist on the model. In this case, we create a
Referenceto the user-provided variables. Names are still used under the hood somewhere.
You can always assume the names exist, although personally I would prefer if we did not rely on them. inputs is a Var whose values are the VarData to be treated as inputs to the grey-box, in order.
There was a problem hiding this comment.
So, inputs.keys() or [v.local_name for v in inputs] then.
There was a problem hiding this comment.
inputs.keys():
- If inputs were provided, these are ints
- If inputs were not provided, these are names
[v.local_name for v in inputs.values()]
- If inputs were provided these are the original var names
- If inputs were not provided, these are, e.g.,
inputs[input_name]
There was a problem hiding this comment.
What is the canonical way to defining inputs and names then?
Names can always be accessed by ExternalGreyBoxModel.input_names. Input variables can always be accessed by ExternalGreyBoxBlockData.inputs.values. These are the canonical ways to access both of these data.
| jacobian_entry = jac[con_idx, var_idx] | ||
|
|
||
| if abs(jacobian_entry) >= jac_tolerance: | ||
| incident_variables.append(self._parent_model.inputs[input_name]) |
There was a problem hiding this comment.
As above, we can't assume that inputs are indexed by names.
| def get_incident_variables( | ||
| self, use_jacobian=False, jac_tolerance=JAC_ZERO_TOLERANCE | ||
| ): | ||
| """ | ||
| Get the variables that are incident on this implicit constraint. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| use_jacobian : bool, optional | ||
| If True, only include variables with non-zero Jacobian entries. | ||
| jac_tolerance : float, optional | ||
| The tolerance below which Jacobian entries are considered zero. | ||
|
|
||
| Returns | ||
| ------- | ||
| list of VarData | ||
| List containing the variables that participate in the expression | ||
|
|
||
| """ | ||
| # There are two ways incident variables could be defined for an implicit constraint: | ||
| # 1) We consider all inputs to the external model to be incident on the implicit constraint | ||
| # 2) We consider only the inputs that have a non-zero Jacobian entry for the implicit constraint | ||
| # to be incident on the implicit constraint | ||
| # Both have their uses, so we will support both. |
There was a problem hiding this comment.
My preference would be to always use the Jacobian and to determine incidence using only the Jacobian's structure. I don't like using numeric values of the Jacobian as that implicitly depends on the input values we're evaluating.
In general, I'm open to other methods of determining variable-constraint incidence. Can you provide an example use-case for considering the variable-constraint incidence to be dense even if the Jacobian is sparse? Does this have to do with decomposability when solving strongly connected components?
There was a problem hiding this comment.
I think there are a few things going on here, and I am not sure there is a single correct answer. Right now, I think there are three possible paths (up from the two I documented in the code), and all three likely have cases where they would be "more correct".
Firstly, a concrete case I can think of where we would want to assume all variables are incident on all constraints: the block-triangularization solver. As the ExternalGreyBoxModel can only be called as a monolithic block, from the perspective of the block-triangularization solver it cannot be decomposed which effectively means all variables are incident on all constraints.
Beyond this however, things get a bit murkier and I'll start with an example.
The main use case I have for ExternalGreyBoxModels is to wrap a procedural thermodynamic flash algorithm which handles four phases (aqueous, organic, vapor and solids). Incidentally, the procedural code returns hard-zero values for non-physical phases (which results in numerical issues I need to deal with, but that is a separate issue). For the ExternalGreyBoxModel however, this raises an issue that numerically, the structure of the model is effectively changing as phases appear and disappear. When I initially implemented this I tried to filter out all those zero values in the wrapper for efficiency, but that mean I ran into issues in the ExternalGreyBoxModel because the shape of the Jacobian was changing from iteration to iteration. My solution was to make the Jacobian fully specified, and to keep all the zero values. However, this means that we cannot deduce the numerical structure of the problem from the Jacobian shape, and hence the idea to use the value of the Jacobian to get the local strucutre.
This would also be needed/useful for detecting local singularities, such as cases where a multiplier variable goes to zero and effectively makes a constraint under/overspecified.
TLDR: I see three possible approaches, each with their own value:
- Assume full incidence: useful for the block-triangularization solver, or users of a grey box who don't want to know about what is inside it.
- Incidence based on Jacobian structure: cheap and unambiguous, but requires that the Jacobian shape reflect the numerical structure of the problem. This is feasible for many problems but starts to fail in cases like mine, and requires the developer of the
ExternalGreyBoxModelto implement the Jacobian correctly. - Incidence based on Jacobian value: requires some inference of what Jacobian value to consider a variable incident on a constraint (and thus scaling dependent), but can handle more complex cases where the numerical structure of the problem is state-dependent.
There was a problem hiding this comment.
In that case, the "gray-box way" to do this would be to have a Jacobian that captures every variable-constraint pair that can possibly be nonzero. But you're saying that that is not so useful for debugging? I'll think about what I'd like to do here.
Incidence analysis was not designed for cases where the incidence is state-dependent, so if we support this here, I'd like to think about supporting this for regular constraints as well. We could generate the entire incidence graph then filter edges (maybe with a new method) if they correspond to a low value?
There was a problem hiding this comment.
I think there are two parts to that.
The first part if whether we can (and should) expect the grey box Jacobian to properly capture "every variable-constraint pair that can possibly be nonzero", as this depends on the developer of the grey box and how they implement the Jacobian. The best answer there might just be to make sure things are documented clearly.
The second part is whether incidence analysis should look for state-dependent structure, which as you note it was not designed to do (although I think it will at least exclude any expression branches with a fixed zero). Your suggestion about having a separate method for that (if we want to do it) is probably the cleanest approach for that.
However, I am not sure that addresses the point about the block-triangularization solver and the distinction between numerically decomposable and functionally decomposable.
There was a problem hiding this comment.
For your use case, do you need the block-triangularization solver to support GreyBox?
There was a problem hiding this comment.
I would need to think about it more, but you have a better understanding of how the everything works and what might be possible. I agree that is it definitely possible in some cases, but I can see edge cases that we would need to take care of.
For example, say we had a grey box that was fundamentally two separate, block-triangularizable sub-systems, A and B (obviously poor design by the developer, but possible). If we ran it through the BT tools and solver, it is possible that it would identify that sub-block A could be solved for before the inputs for sub-block B were known (numerically at least), meaning that if we were to call the grey box then we could have undefined behaviour when the grey box solves sub-block B (e.g. what happens if the inputs have value of None? What if the current values are infeasible for sub-block B?).
However, as I am not expecting the use the block-triangularization solver myself, I would be fine with deferring this if we think it needs more thought or effort.
There was a problem hiding this comment.
In this case, B would need to be initialized with well-defined inputs. (I don't think this is a restriction. I think A's inputs need to be valid values at the start of the solve as well.) It doesn't matter if the initial values are infeasible, we just ignore B's residual while solving A. If A's outputs end up making B infeasible, that's a general problem that can happen in any BT algorithm (and in my opinion is a useful feature of the algorithm).
But this doesn't need to happen in this PR. We can always raise NotImplementedError for now. I'm just looking for ways to break this apart and limit the functionality I promise to support moving forward.
There was a problem hiding this comment.
Agreed - should I add a check for ExternalGreyBoxBlocks in the BT solver and raise an exception then?
There was a problem hiding this comment.
Leave it for now. First I want to (a) see what is required to support user-provided inputs and outputs and (b) decide what to do about state-dependent Jacobian structure in the GB.
There was a problem hiding this comment.
I think state-dependent structure is probably something for a separate PR too - as you noted the current tools do not consider state dependence on a pure Pyomo model, so grey box models should behave the same way. If we go with that, then all we need for incidence analysis is to look at the shape of the Jacobian that is returned which should be easy.
User inputs should also just be a matter of finding the right way to get the index-name-object mappings.
|
@Robbybp I implemented an approach to use the Jacobian shape to determine incidence which turned out to be fairly simple, and also addressed the issue with input names. However, using your test case revealed two issues:
|
|
Digging a little deeper, it is actually a bit more complicated. The main error is coming from the fact that the method to get the Jacobain requires the This means the issue really lies in the code for the grey box, however all the examples are written this way. |
|
@Robbybp I just pushed my latest version with the fixes for using Jacobian shape and allowing external input and output variables. |
Co-authored-by: Copilot <copilot@github.com>
…in tests. Co-authored-by: Copilot <copilot@github.com>
…ing most tests Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
@Robbybp I believe I addressed all your inline comments. If you could help with the additional tests it would be appreciated, as I don't have a lot of time right now. |
|
@andrewlee94, @Robbybp: FYI: We are adopting a new review process where we convert PRs that are "waiting on the author" back to "draft" (to signal the PR state to both the author and the dev team). Please use the "Ready for review" button to signal the developers when it is time to get the PR back into the review queue. |
| @@ -0,0 +1,185 @@ | |||
| # ___________________________________________________________________________ | |||
| # | |||
| # Pyomo: Python Optimization Modeling Objects | |||
There was a problem hiding this comment.
Please check / update the copyright statement (this is last year's).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3870 +/- ##
==========================================
+ Coverage 89.93% 90.07% +0.13%
==========================================
Files 902 904 +2
Lines 106454 107968 +1514
==========================================
+ Hits 95744 97253 +1509
- Misses 10710 10715 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
A few quick comments. I also opened a PR where I reimplemented get_variables_incident_to_constraint here: andrewlee94#1
| self.assertIs(bt_cons[0][0], m.con1) | ||
| self.assertIs(bt_cons[1][0], m.con2) | ||
| self.assertIs(bt_cons[2][0], m.con3) |
There was a problem hiding this comment.
The order of these three constraints is not uniquely defined by the block-triangular decomposition.
| prefix = self._block.getname(fully_qualified=True) | ||
| self._constraint_names = [ | ||
| '{}.{}'.format(prefix, nm) | ||
| for nm in self._ex_model.equality_constraint_names() | ||
| ] | ||
| output_var_names = [ | ||
| self._block.outputs[k].getname(fully_qualified=False) | ||
| for k in self._block.outputs | ||
| ] | ||
| self._constraint_names.extend( | ||
| [ | ||
| '{}.output_constraints[{}]'.format(prefix, nm) | ||
| for nm in self._ex_model.output_names() | ||
| ] | ||
| ) | ||
| # Collect implicit constraints. | ||
| self._constraint_names = [] | ||
| self._constraint_datas = [] | ||
| for c in self._block.component_data_objects( | ||
| ExternalGreyBoxConstraint, active=True, descend_into=False | ||
| ): | ||
| self._constraint_names.append(c.getname(fully_qualified=True)) | ||
| self._constraint_datas.append(c) |
There was a problem hiding this comment.
It's really nice that we get the constraint's name with c.name, but this change is not backwards compatible. Can this be reverted?
There was a problem hiding this comment.
I believe it can be reverted, however it does result in different behaviour for equality and output constraint naming conventions. Equality constraints will be named using their specified name (e.g., ex_block.residual_0), but output constraint will use the name from the external model (e.g., ex_block.output_constraint[out_var_name]).
There was a problem hiding this comment.
This also flows over into how the duals get set.
There was a problem hiding this comment.
Yep, I'm not thrilled about these different naming conventions, but I'd like to preserve current behavior for now. If we want to make a breaking change to a more consistent naming convention, we can do it in another PR.
| "ex_block.residual_0", | ||
| "ex_block.residual_1", | ||
| "ex_block.eq_constraints[residual_0]", | ||
| "ex_block.eq_constraints[residual_1]", |
There was a problem hiding this comment.
I believe these changes are only necessary because you switch from using e.g., egbm.equality_constraint_names to con.name. I think there's an argument for making this change eventually, but I would consider this breaking and would prefer not to make this change right now.
Updates to implicit-grey box integration
|
@andrewlee94 @Robbybp is this ready for final reviews? |
I still want to go through the ImplicitConstraint implementation in more detail, but I don't think that should stop anybody else from reviewing. |
@Robbybp will you have time to look at this PR this week. We would like to get it into the Pyomo May release (which has already been pushed back by a week). The dev team is starting to review it also. |
| # # Collect implicit constraints. | ||
| # self._constraint_names = [] | ||
| # self._constraint_datas = [] | ||
| # for c in self._block.component_data_objects( | ||
| # ExternalGreyBoxConstraint, active=True, descend_into=False | ||
| # ): | ||
| # self._constraint_names.append(c.getname(fully_qualified=True)) | ||
| # self._constraint_datas.append(c) | ||
|
|
| v.set_value(1.0) | ||
|
|
||
|
|
||
| def test_with_custom_input_names(): |
There was a problem hiding this comment.
This awkwardly switches between unittest and pytest style within the same file. It's not an issue, but it looks weird.
|
|
||
|
|
||
| @skip_implicit_constraint_construction | ||
| def test_component_data_objects_with_EGBC(): |
There was a problem hiding this comment.
Similar to previous comment - switched from unittest to pytest style. Not a problem, but weird.
| # ___________________________________________________________________________ | ||
| # | ||
| # Pyomo: Python Optimization Modeling Objects | ||
| # Copyright (c) 2008-2025 |
There was a problem hiding this comment.
+1 - old copyright statement.
| scipy, | ||
| scipy_available, | ||
| ) | ||
| from pyomo.common.dependencies.scipy import sparse as spa |
There was a problem hiding this comment.
Probably doesn't make that much of a difference but in test_external_grey_box_model_with_constraints.py, you checked availability before actually attempting to import a specific part of scipy:
from pyomo.contrib.pynumero.dependencies import (
numpy as np,
numpy_available,
scipy_available,
)
if not (numpy_available and scipy_available):
raise unittest.SkipTest("Pynumero needs scipy and numpy to run NLP tests")
from scipy.sparse import coo_matrix
Do you think it's worth redoing that for this file?
| # m.scaling_factor[m.obj] = 0.1 # scale the objective | ||
| m.scaling_factor[m.egb.inputs['Pin']] = 1.1 # scale the variable | ||
| m.scaling_factor[m.egb.inputs['c']] = 1.2 # scale the variable | ||
| m.scaling_factor[m.egb.inputs['F']] = 1.3 # scale the variable | ||
| # m.scaling_factor[m.egb.inputs['P1']] = 1.4 # scale the variable | ||
| m.scaling_factor[m.egb.inputs['P3']] = 1.5 # scale the variable | ||
| m.scaling_factor[m.egb.outputs['P2']] = 1.6 # scale the variable | ||
| m.scaling_factor[m.egb.outputs['Pout']] = 1.7 # scale the variable | ||
| # m.scaling_factor[m.hin] = 1.8 | ||
| m.scaling_factor[m.hout] = 1.9 | ||
| # m.scaling_factor[m.incon] = 2.1 |
There was a problem hiding this comment.
Several of these commented out lines look like they might be residual from earlier iterations / playing with test values. Remove?
|
|
||
|
|
||
| class TestGreyBoxObjectives(unittest.TestCase): | ||
| @unittest.skipIf(not cyipopt_available, "CyIpopt needed to run tests with solve") |
There was a problem hiding this comment.
You can just have one check for cyipopt_available at the top of this test class.
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Overall, this looks reasonable. Several questions, though...
| if constraint.ctype is ExternalGreyBoxConstraint: | ||
| return constraint.body.get_incident_variables() | ||
| else: | ||
| return get_incident_variables(constraint.body, **kwds) |
There was a problem hiding this comment.
Why does this need a special override (couldn't this be handled in get_incident_variables)?
There was a problem hiding this comment.
I thought it was more natural to have a function that would handle "any" constraint rather than adding more logic to get_incident_variables, which is documented to operate on expressions.
There was a problem hiding this comment.
My thought process was to make the EGBBody look more like an expression so that the special case handling was unnecessary.
For example, if EGBBody had an args property that returned the input / output variables, then the existing code should be able to handle it, right?
| JAC_ZERO_TOLERANCE = 1e-8 | ||
|
|
||
|
|
||
| class EGBConstraintBody: |
There was a problem hiding this comment.
Should this use either NumericValue (or more likely NumericExpression) as a base class? This is effectively representing an anonymous named expression....
| """ | ||
|
|
||
| def __init__(self, parent_model, implicit_constraint_id): | ||
| self._parent_model = parent_model |
There was a problem hiding this comment.
_parent_model is confusing. This is a pointer to the external grey box, and not the Pyomo "model" (root block). Maybe _egb?
|
|
||
| def __init__(self, parent_model, implicit_constraint_id): | ||
| self._parent_model = parent_model | ||
| self._implicit_constraint_id = implicit_constraint_id |
There was a problem hiding this comment.
_implicit seems superfluous (it's not an implicit constraint - it is a "full" / "real" constraint that is seen by the solver).
Maybe just _constraint_id?
| self._ext_output_idx = None | ||
| self._ext_eq_cons_idx = None |
| def deactivate(self): | ||
| """Raise a TypeError, as ExternalGreyBoxConstraints cannot be activated or deactivated.""" | ||
| # Refer back to the activate method to ensure message consistency. | ||
| self.activate() |
There was a problem hiding this comment.
I think this is potentially dangerous, as it could interact in unexpected ways with, e.g. derived classes. I would prefer this just raise the exception.
There was a problem hiding this comment.
Similar to my comment above, I would go a step further and say just omit these methods.
| # Check for normal Constraint arguments, and raise a TypeError if found | ||
| rule = kwargs.pop('rule', None) | ||
| expr = kwargs.pop('expr', None) | ||
|
|
||
| if rule is not None: | ||
| raise TypeError( | ||
| "The 'rule' argument is not supported by ExternalGreyBoxConstraint. " | ||
| "Use the 'implicit_constraint_id' argument instead." | ||
| ) | ||
| if expr is not None: | ||
| raise TypeError( | ||
| "The 'expr' argument is not supported by ExternalGreyBoxConstraint. " | ||
| "ExternalGreyBoxConstraints do not have explicit expressions." | ||
| ) |
There was a problem hiding this comment.
This is not necessary, as the base class will raise an error on non-empty kwargs.
| kwargs.setdefault('ctype', ExternalGreyBoxConstraint) | ||
| IndexedComponent.__init__(self, *args, **kwargs) | ||
|
|
||
| # Validate implicit_constraint_id |
There was a problem hiding this comment.
This logic should almost certainly be moved to construct().
Also, why is this here and not in the Data class? When would the container ever have a constraint_id?
| implicit_constraint_id | ||
| The identifier for this implicit constraint or output variable |
There was a problem hiding this comment.
Why is this part of the container and not the Data?
|
|
||
| # Check to ensure we have some non-zeroes. | ||
| # If not, we still need to maintain consistent internal state and maps so sum() returns a valid empty matrix. | ||
| if len(nz_tuples) == 0: |
There was a problem hiding this comment.
| if len(nz_tuples) == 0: | |
| if not nz_tuples: |
(We don't need the actual length, just if it is non-empty.)
| # | ||
| # Additional contributions Copyright (c) 2026 OLI Systems, Inc. | ||
| # ___________________________________________________________________________________ |
There was a problem hiding this comment.
Since this file has no additional changes, can this be removed?
| self.assertIn('d scaling provided', solver_trace) | ||
| # x_order: ['egb.inputs[F]', 'egb.inputs[P1]', 'egb.inputs[P3]', 'egb.inputs[Pin]', 'egb.inputs[c]', 'egb.outputs[P2]', 'egb.outputs[Pout]', 'mu'] | ||
| # c_order: ['ccon', 'pcon', 'pincon', 'egb.pdrop1', 'egb.pdrop3', 'egb.output_constraints[P2]', 'egb.output_constraints[Pout]'] | ||
| # c_order: ['ccon', 'pcon', 'pincon', 'egb.eq_constraints[pdrop1]', 'egb.eq_constraints[pdrop3]', 'egb.output_constraints[P2]', 'egb.output_constraints[Pout]'] |
There was a problem hiding this comment.
Can you revert this just to clean up the diff slightly.
| # Compatibility API for PyomoNLP - this is only a partial implementation | ||
| def get_pyomo_variables(self): | ||
| return self._pyomo_model_var_datas | ||
|
|
||
| def get_pyomo_constraints(self): | ||
| return list(self._pyomo_model_constraint_names_to_datas.values()) | ||
|
|
||
| def get_pyomo_equality_constraints(self): | ||
| return [c for c in self.get_pyomo_constraints() if c.equality] | ||
|
|
||
| def get_pyomo_inequality_constraints(self): | ||
| return [c for c in self.get_pyomo_constraints() if not c.equality] | ||
|
|
||
| def get_primal_indices(self, var): | ||
| # get the name of the variable | ||
| var_name = var.getname(fully_qualified=True) | ||
| # get the index of the variable in the primals | ||
| try: | ||
| return self._primals_names.index(var_name) | ||
| except ValueError: | ||
| raise ValueError(f'Variable {var_name} not found in primals.') |
There was a problem hiding this comment.
I assume these are added for compatibility with IncidenceGraphInterface? Or is there another reason beyond this?
| def constraint_datas(self): | ||
| return list(self._constraint_datas) | ||
|
|
| ) | ||
|
|
||
|
|
||
| class TestExternalGreyBoxAsNLP(unittest.TestCase): |
There was a problem hiding this comment.
I'm not sure I understand the purpose of any of the tests in this file. The name suggests that we are testing PyomoNLPWithGreyBoxBlocks in the case where the underlying ExternalGreyBoxBlock has constructed ExternalGreyBoxConstraints.
- This PR makes only minor changes to
PyomoNLPWithGreyBoxBlocks, so I don't see the need for extensive testing of its variables, constraints, Jacobian, etc. - Since we now always construct the constraints, the existing tests for
PyomoNLPWithGreyBoxBlockstest this already, right?
If anything, I think this file should just contain unit tests for the methods added to ducktype PyomoNLP (or these tests can go in the existing PyomoNLPWithGreyBoxNLP test file).
| @property | ||
| def lb(self): | ||
| """float : the value of the lower bound of a ExternalGreyBoxConstraint expression. | ||
|
|
||
| Implicit constraints always have a lower bound of 0. | ||
| """ | ||
| return 0.0 | ||
|
|
||
| @property | ||
| def ub(self): | ||
| """float : the value of the upper bound of a ExternalGreyBoxConstraint expression. | ||
|
|
||
| Implicit constraints always have an upper bound of 0. | ||
| """ | ||
| return 0.0 | ||
|
|
||
| @property | ||
| def equality(self): | ||
| """bool : True. ExternalGreyBoxConstraints are always equalities.""" | ||
| return True |
There was a problem hiding this comment.
Can some of these properties be removed (for now)? I think most of them are not necessary for the functionality this PR is adding. My understanding is that subclassing Constraint (instead of ducktyping) has the following advantages:
- Access to
.pprint .namebehaves like a normal Pyomo name- Compatibility with
component_data_objects
Everything else, to me, has marginal value for right now. My suggestion is to remove as many properties and methods as possible from this file. My rationale is that it will be relatively easy to add these later if we have a good reason to, but relatively annoying to change if we add them now.
| def deactivate(self): | ||
| """Raise a TypeError, as ExternalGreyBoxConstraints cannot be activated or deactivated.""" | ||
| # Refer back to the activate method to ensure message consistency. | ||
| self.activate() |
There was a problem hiding this comment.
Similar to my comment above, I would go a step further and say just omit these methods.
| m.F.fix(1) | ||
|
|
||
| # Check Dulmage-Mendelsohn partitioning of the incidence graph | ||
| igraph = IncidenceGraphInterface(m, include_inequality=False) |
There was a problem hiding this comment.
How do these tests differ from those in the Incidence Analysis tests? I would lean towards putting all of the Incidence Analysis-Grey box integration tests in Incidence Analysis.
6 participants
Fixes None.
Summary/Motivation:
Pyomo currently has no way of explicitly representing the constraints within an ExternalGreyBoxBlock, which means that they cannot be seen by diagnostics tool (such as Incidence Analysis). This PR adds a new optional ExternalGreyBoxConstraint component which duck-types Constraint which can be added to ExternalGreyBoxBlocks to provide a representation of the grey box constraints. The incidence analysis and PyomoNLPWithGreyBoxBlocks code has been updated to recognise and understand these components.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: