Simple disjunctive transform#3982
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3982 +/- ##
==========================================
- Coverage 90.12% 90.02% -0.11%
==========================================
Files 909 918 +9
Lines 108561 109888 +1327
==========================================
+ Hits 97836 98922 +1086
- Misses 10725 10966 +241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
emma58
left a comment
emma58
left a comment
There was a problem hiding this comment.
@cjohnston1 , this is looking good! A couple more major things to change though:
0) Please switch to using the GDP Tree for all the nested cases--that should completely eliminate a lot of your methods for handling nested structures and simplify some other things too.
- Can you please use a private data block for the mapping from the transformed to the original model? Look to bigm for an example of that.
- Only create one transformation block per parent block and put multiple transformed Disjunctions on the transformation blocks.
After you do this, I can take another look.
|
|
||
| @TransformationFactory.register( | ||
| 'gdp.simple_disjunction', | ||
| doc="Relax selected Disjunctions by building, for each one, a 'simple' " |
There was a problem hiding this comment.
This transformation is not a relaxation, I don't think. I think I'd call this "reformulation"
| "the corresponding original Disjunct.", | ||
| ) | ||
| class SimpleDisjunctionTransformation(Transformation): | ||
| """Create a relaxation of one or more Disjunctions as *simple* Disjunctions. |
| disjunction, build_expression, selected_constraints | ||
| ) | ||
|
|
||
| def _get_disjunctions_to_transform(self, instance, targets): |
There was a problem hiding this comment.
I'd vote for not making this a method and just moving the logic it into the method above: You can just transform Disjunctions as you find them and not have to build a separate list of them (which could be quite long for big models.)
| ) | ||
| return disjunctions | ||
|
|
||
| def _gather_disjunctions(self, block): |
There was a problem hiding this comment.
You can get this from the GDP Tree when you switch to it.
| if _parent_disjunct(disjunction) is not None: | ||
| raise GDP_Error( | ||
| "Disjunction '%s' is nested in another Disjunct. This " | ||
| "transformation does not create simple disjunctions from nested " | ||
| "Disjunctions." % disjunction.name | ||
| ) | ||
| nested = self._nested_disjunction_owner(disjunction) | ||
| if nested is not None: |
There was a problem hiding this comment.
Let's move all this logic to use the GDP Tree.
| if not isinstance(constraint, ConstraintData): | ||
| raise GDP_Error( | ||
| "An object selected for Disjunct '%s' in " | ||
| "'selected_constraints' is not a Constraint. Expected a " | ||
| "ConstraintData, but got an object of type %s." | ||
| % (disjunct.name, type(constraint).__name__) | ||
| ) |
There was a problem hiding this comment.
In the spirit of duck-typing, I think you can just assume it's a Constraint until it fails to act like one.
| if type(src) is not weakref_ref: | ||
| raise GDP_Error( | ||
| "It appears that '%s' is not a simple disjunction generated by " | ||
| "the 'gdp.%s' transformation. No source disjunction found." | ||
| % (simple_disjunction.name, self.transformation_name) | ||
| ) | ||
| return src() |
There was a problem hiding this comment.
I would neither use a weakref here nor validate what you got... If it was there, it should be the right thing, so in this case if a user hits this error, you are complaining to them about your bug. You should however, probably raise a helpful error if you don't find simple_disjunction in your map. You can look at other transformations for an example of that.
| ) | ||
| return simple | ||
|
|
||
| def get_src_disjunction(self, simple_disjunction): |
There was a problem hiding this comment.
You'll need to update this function when you switch to private data classes.
| from pyomo.gdp.plugins import simple_disjunction_transform | ||
|
|
||
|
|
||
| class CommonModels: |
There was a problem hiding this comment.
There are a bunch of GDP models for testing in tests/models.py. I was trying at one point to avoid a proliferation of very similar models... I don't really have strong feelings about it, but your nested one might actually be very similar to some that are already there. If it's hard to switch don't worry, but maybe put these in that file.
| trans = self.get_transformation_block(m) | ||
| self.assertIsNotNone(trans) | ||
| simple = trans.simple_disjunction | ||
| self.assertIsInstance(simple, Disjunction.__mro__[0]) |
There was a problem hiding this comment.
Disjunction.__mro__[0] is the class itself, I think. This should just be Disjunction.
4 participants
Fixes # .
Summary/Motivation:
This PR is a draft meant to facilitate conversation and is not ready to be reviewed and committed.
This PR is a draft of the simple disjunctive transformation module. This creates a new transformed disjunction which has a single inequality per disjunct for either user given disjunctions or all disjunctions on a model if no input is given. The transformation can be done in multiple ways which are selected by the user. This is motivated by the development of another module which can generate a family of inequalities for these transformed disjunctive constraints.
Changes proposed in this PR:
AI-Use Disclosure
or
AI tools contributed to the development of this PR
Review process (select ONE):
Notes for reviewers (optional):
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: