Adding (reversible) gdp.transform_current_disjunctive_logic transformation#2809
Adding (reversible) gdp.transform_current_disjunctive_logic transformation#2809emma58 merged 25 commits intoPyomo:mainfrom
gdp.transform_current_disjunctive_logic transformation#2809Conversation
…ng the current values, not requiring they be fixed. Adding some tests
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Cool PR. I see how this would simplify things for GDPOpt. I see no issues with it.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2809 +/- ##
==========================================
- Coverage 87.83% 87.63% -0.20%
==========================================
Files 770 771 +1
Lines 89642 89996 +354
==========================================
+ Hits 78733 78872 +139
- Misses 10909 11124 +215
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
…wallowed by the base class anyway, so they couldn't be functional.
…nsformationToken, but I broke it
…we were only looking at active ones.
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Overall, this looks reasonable, but there is a bug that needs to be addressed before merging (supporting transforming partial models)
| for disj in true_disjunctions: | ||
| reverse_dict[disj] = (disj.indicator_var.fixed, disj.indicator_var.value) | ||
| disj.indicator_var.fix(True) | ||
| disj.parent_block().reclassify_component_type(disj, Block) |
There was a problem hiding this comment.
I worry as this reclassifies all Disjuncts in an indexed Disjunct, whether nor not all the disjuncts state is actually determined by the indicator_var (e.g., only transforming part of a model)
There was a problem hiding this comment.
To document: We are currently resolving this by not allowing partial transformation. We will complain if we cannot fully transform any Disjunction in the active tree specified by targets and we will complain if we reclassifying a DisjunctData that is in a container with other active DisjunctDatas that the transformation doesn't touch.
…but still needs to check for indexing in the reverse transformation
…dDisjuncts get partially transformed and throwing an error
| parent_block = disj.parent_block() | ||
| parent_block.reclassify_component_type(disj, Block) |
There was a problem hiding this comment.
The only concern I have here is that if an indexed Disjunct has a member that is going to be fixed True (and thus reclassified as a Block) has any peer Disjuncts that are not in a Disjunction that can be transformed, then those disjuncts are also going to be transformed as active Blocks. That is:
m.d = Disjunct([1,2,3], rule=...)
m.e = Disjunct([1,2], rule=...)
m.disj = Disjunction(expr=[m.d[1], m.e[1])
m.d[1].indicator_var = False
TransformationFactory('gdp.transform_current_disjunctive_state').apply_to(m)...Now m.e[2] is an active block in the model!
There was a problem hiding this comment.
I think this is actually okay--we're comparing the GDP hierarchy with the component hierarchy as we do this transformation, so if there are components that we didn't catch as we transform GDP stuff, we throw an error at the end since we may have inadvertently reclassified them. So your example above throws this error:
Traceback (most recent call last):
File "/home/esjohn/src/pyomo/thing.py", line 10, in <module>
TransformationFactory('gdp.transform_current_disjunctive_state').apply_to(m)
File "/home/esjohn/src/pyomo/pyomo/core/base/transformation.py", line 77, in apply_to
reverse_token = self._apply_to(model, **kwds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/esjohn/src/pyomo/pyomo/gdp/plugins/transform_current_disjunctive_state.py", line 146, in _apply_to
raise GDP_Error(
pyomo.gdp.disjunct.GDP_Error: Found active Disjuncts on the model that were not included in any Disjunctions:
d[3], d[2], e[2]
Please deactivate them or include them in a Disjunction.
5 participants
Related to #277
Summary/Motivation:
The current
gdp.fix_disjunctstransformation guarantees that it transforms a GDP all the way to a MI(N)LP. Because this means it needs to call another transformation to transform any LogicalConstraints, this makes it difficult to add a "reverse" transformation to undo the reclassification of the Disjuncts (which is a useful thing for initializing nonlinear GDP models for solving with GDPopt, etc.) Instead of trying to deprecate the going-to-MINLP guarantee, this PR adds a new transformation,gdp.transform_current_disjunctive_statethat doesn't even promise to transform all the GDP components: It just goes through all of the Disjunctions, and if any have enough indicator_vars fixed to determine which Disjunct(s) will be selected, it reclassifies the Disjuncts and activates/deactivates accordingly. It currently does nothing with LogicalConstraints, though in the future it would be useful if it took them into account in the logic to decide what Disjuncts will be selected.Changes proposed in this PR:
Transformationbase class so thatapply_tohas a return value, which is assumed to be a token containing the right information so it can be passed into areverseargument in order to undo the transformation.gdp.transform_current_disjunctive_logictransformationLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: