Simple disjunctive transform by cjohnston1 · Pull Request #3982 · Pyomo/pyomo

cjohnston1 · 2026-06-22T18:08:31Z

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:

Adds the simple disjunction transform module
Adds tests for the simple disjunction transform module.

AI-Use Disclosure

AI tools were NOT used during the preparation of this PR

or

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:

I agree my contributions are submitted under the BSD license.
I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

…tion_transform.py

codecov · 2026-06-22T22:53:52Z

Codecov Report

❌ Patch coverage is 93.02326% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (44c4697) to head (76293ed).
⚠️ Report is 136 commits behind head on main.

Files with missing lines	Patch %	Lines
pyomo/gdp/plugins/simple_disjunction_transform.py	93.02%	12 Missing ⚠️

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

Flag	Coverage Δ
builders	`29.10% <25.58%> (-0.01%)`	⬇️
default	`86.12% <93.02%> (?)`
expensive	`35.11% <25.58%> (?)`
linux	`85.90% <93.02%> (-3.72%)`	⬇️
linux_other	`85.90% <93.02%> (-1.73%)`	⬇️
oldsolvers	`28.03% <25.58%> (-0.01%)`	⬇️
osx	`83.12% <93.02%> (+0.06%)`	⬆️
win	`85.42% <93.02%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

emma58

@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.

emma58 · 2026-06-22T18:16:05Z

+
+@TransformationFactory.register(
+    'gdp.simple_disjunction',
+    doc="Relax selected Disjunctions by building, for each one, a 'simple' "


This transformation is not a relaxation, I don't think. I think I'd call this "reformulation"

emma58 · 2026-06-22T18:16:48Z

+    "the corresponding original Disjunct.",
+)
+class SimpleDisjunctionTransformation(Transformation):
+    """Create a relaxation of one or more Disjunctions as *simple* Disjunctions.


emma58 · 2026-06-22T19:45:43Z

+                disjunction, build_expression, selected_constraints
+            )
+
+    def _get_disjunctions_to_transform(self, instance, targets):


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.)

emma58 · 2026-06-22T19:46:42Z

+                )
+        return disjunctions
+
+    def _gather_disjunctions(self, block):


You can get this from the GDP Tree when you switch to it.

emma58 · 2026-06-22T19:47:10Z

+        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:


Let's move all this logic to use the GDP Tree.

emma58 · 2026-06-26T20:42:22Z

+            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__)
+                )


In the spirit of duck-typing, I think you can just assume it's a Constraint until it fails to act like one.

emma58 · 2026-06-26T20:47:25Z

+        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()


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.

emma58 · 2026-06-26T20:53:58Z

+from pyomo.gdp.plugins import simple_disjunction_transform
+
+
+class CommonModels:


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.

emma58 · 2026-06-26T20:58:20Z

+        trans = self.get_transformation_block(m)
+        self.assertIsNotNone(trans)
+        simple = trans.simple_disjunction
+        self.assertIsInstance(simple, Disjunction.__mro__[0])


Disjunction.__mro__[0] is the class itself, I think. This should just be Disjunction.

…ed disjunctions, updated descriptions.

emma58

@cjohnston1 this is getting close! I'm sorry for not thinking of this earlier, but I have one more concern: This is not yet a "transformation" in the sense of the resulting model being equivalent to the original. Specifically, we aren't mapping the new simple disjunction indicator_vars to the originals, and, even when we add that, skipping Disjuncts without any active Constraints is not the right approach because the simple disjunction still needs to be allowed to choose "nothing" in that case just like the original. I'm okay with raising an error in that case because it's kind of insane: optimization will always choose to not constrain something if it can--that at least has to be an optimal indicator variable assignment. But the fundamental thing is that we should have whatever we do here not change the model.

emma58 · 2026-07-18T14:28:53Z

+    __slots__ = ('src_disjunction',)
+
+    def __init__(self):
+        self.src_disjunction = ComponentMap()


Unless you need this to be ordered, you can just use a dictionary, since you're mapping Disjunctions to each other (and they're hashable). It'll be a little faster.

emma58 · 2026-07-18T14:34:53Z

+def _as_constraint_list(value):
+    """Normalize a ``selected_constraints`` value into a list of Constraints.
+
+    Accepts a single ConstraintData, an indexed Constraint container (expanded
+    into its members), or any iterable of Constraints. Anything else is wrapped
+    in a single-element list so that validation can reject it with a clear
+    message. Returning a list (rather than a single Constraint) keeps the data
+    structure ready for selection methods that aggregate several Constraints
+    into one.
+    """
+    if isinstance(value, ConstraintData):
+        return [value]
+    if isinstance(value, ComponentBase) and value.ctype is Constraint:
+        return list(value.values())
+    if isinstance(value, (list, tuple, set, ComponentSet)):
+        return list(value)
+    return [value]


You only check that singletons are Constraints here, not iterables. So I think you can just use the target_list function from

pyomo/pyomo/core/util.py

Line 240 in b953cf9

def target_list(x):

here. I think that's alright (and reduces duplicated code) since you'll crash if something fails to behave like a Constraint later anyway.

emma58 · 2026-07-18T14:43:31Z

+    if isinstance(arg, (ComponentMap, dict)):
+        items = arg.items()
+    else:
+        try:
+            items = dict(arg).items()
+        except (TypeError, ValueError):
+            raise ValueError(
+                "Expected a dict or ComponentMap mapping Disjuncts to "
+                "Constraints for 'selected_constraints', but received an object "
+                "of type %s" % (type(arg).__name__,)
+            )
+    result = ComponentMap()
+    for disjunct, value in items:
+        result[disjunct] = _as_constraint_list(value)
+    return result


I think this is more complicated than it needs to be and you probably don't need this method at all. I would just assume that arg has the items method later on in the code: If it doesn't that's the error Python will throw, which should be clear enough in combination with the fact that you have documentation elsewhere of what the input should be. In addition to duck-typing, my motive here is that this function has a side effect that you always copy the dictionary, which could get expensive for large models.

emma58 · 2026-07-18T14:45:18Z

+    Constraint derived from the Disjunct it was generated from, and reuses the
+    original model (problem) variables.


Suggested change

Constraint derived from the Disjunct it was generated from, and reuses the

original model (problem) variables.

Constraint from the Disjunct it was generated from.

emma58 · 2026-07-18T14:47:02Z

+    parent Block. The new Disjuncts get their own indicator variables, so the
+    simple Disjunction is an independent component that the caller may transform
+    or otherwise use however they see fit.


Whoops, sorry I didn't think of this sooner, but we probably should add LogicalConstraints making an equivalence between the simple disjunction's indicator_vars and the old guy's. Else this really isn't a transformation: The transformed model isn't equivalent to the original without that.

emma58 · 2026-07-18T15:02:41Z

+            if not disjunction.active:
+                # Discovery only turns up active Disjunctions, so an inactive one
+                # here was named explicitly.
+                if disjunction in explicit:


You can skip creating explicit and skip all these checks: Let's treat everything the same.

emma58 · 2026-07-18T15:03:33Z

+        else:
+            for t in targets:
+                if t.ctype is Disjunction:
+                    explicit.update(t.values() if t.is_indexed() else (t,))


I think the only thing you need here is a check if the target is inactive, because that is an error. But you don't need to build explicit. (Honestly, we should probably have an option to check for active stuff when we build the gdp_tree... But we don't right now.)

emma58 · 2026-07-18T15:07:18Z

+    @staticmethod
+    def _nested_disjunction_owner(disjunction, gdp_tree):
+        # Return the first Disjunct of `disjunction` that owns a nested
+        # Disjunction, or None. In the GDP tree, a Disjunct with a nested
+        # Disjunction has that Disjunction as a child, so it is not a leaf.
+        for disjunct in disjunction.disjuncts:
+            if disjunct in gdp_tree.vertices and not gdp_tree.is_leaf(disjunct):
+                return disjunct
+        return None


You don't need this method: Just ask for gdp_tree.parent(disjunction). (That will return None if disjunction is a root.)

emma58 · 2026-07-18T21:33:41Z

+            if expression is None:
+                logger.debug(
+                    "Disjunct '%s' has no active constraints to select, so it "
+                    "is skipped in the simple disjunction generated from "
+                    "Disjunction '%s'." % (disjunct.name, disjunction.name)
+                )
+                continue


Sorry I didn't catch this before yet either, but shouldn't you still create an empty Disjunct in the simple disjunction so that the transformed model will be equivalent to the original? If that messes up the constraint generation, maybe we put a trivial constraint on it, like just a redundant bounds constraint or something?

emma58 · 2026-07-18T21:40:20Z

+        if len(sources) != 1:
+            raise GDP_Error(
+                "Disjunct '%s' (in Disjunction '%s') was assigned %d constraints, "
+                "but the current selection methods keep exactly one constraint "
+                "per Disjunct. (Aggregating several constraints into one is not "
+                "yet implemented.)" % (disjunct.name, disjunction.name, len(sources))


Oh, just kidding, I take it back. You could get here with bad user input to selected_constraints right?

Johnston added 2 commits June 5, 2026 14:54

Added simple disjunctive transform

24b9c3d

Add gdp.simple_disjunction transformation and gdp.test_simple_disjunc…

76293ed

…tion_transform.py

jsiirola added the AT: PRE-TEST INSPECTED label Jun 22, 2026

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Jun 22, 2026

emma58 requested changes Jun 26, 2026

View reviewed changes

Responding to comments on my pr, changed to use GDP for checking nest…

edeefd5

…ed disjunctions, updated descriptions.

emma58 reviewed Jul 18, 2026

View reviewed changes

		from pyomo.gdp.plugins import simple_disjunction_transform


		class CommonModels:

		Constraint derived from the Disjunct it was generated from, and reuses the
		original model (problem) variables.

	Constraint derived from the Disjunct it was generated from, and reuses the
	original model (problem) variables.
	Constraint from the Disjunct it was generated from.

Uh oh!

Conversation

cjohnston1 commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

AI-Use Disclosure

Legal Acknowledgement

Uh oh!

codecov Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

emma58 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emma58 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cjohnston1 commented Jun 22, 2026 •

edited

Loading

codecov Bot commented Jun 22, 2026 •

edited

Loading