Convert implicit sets created by IndexedComponents to "anonymous" sets
#3075
Conversation
…index.flatten state is restored)
… AbstractScalarVar
jsiirola
changed the title
IndexedComponents to "anonymous" sets
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3075 +/- ##
==========================================
- Coverage 88.28% 88.28% -0.01%
==========================================
Files 832 832
Lines 92307 92392 +85
==========================================
+ Hits 81495 81566 +71
- Misses 10812 10826 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| 2 Var Declarations | ||
| x : Size=12, Index=x_index | ||
| x : Size=12, Index={Harlingen, Memphis, Ashland}*{NYC, LA, Chicago, Houston} |
There was a problem hiding this comment.
I like this change in the string representation to use the Set data as the name when there are only a few data elements but this output could quickly lose its human readability when the sets are large. What do you think about concatenating the data when the string representation exceeds a certain number of characters to guard against this case? The output could look something like:
Index={Harlingen, Memphis, Ashland, ...}*{NYC, LA, Chicago, Houston, ...}
There was a problem hiding this comment.
I've been wanting to to that for a long time (and not just here, but in expressions, too), but that would require a significant redesign of the way we convert things to strings. I could hack something in here, but wonder if we should do it as a separate PR.
(The challenge is this is really part of converting expressions to strings, and strings are built from the leaf nodes up to the root node. So, you may not know that the result is "too long" until you get up the expression tree , but then need to go back down the tree and truncate things closer to the leaves)
| @@ -642,7 +642,7 @@ def Reference(reference, ctype=NOTSET): | |||
| ... | |||
| >>> m.r3 = Reference(m.b[:].x[:]) | |||
| >>> m.r3.pprint() | |||
| r3 : Size=4, Index=r3_index, ReferenceTo=b[:].x[:] | |||
| r3 : Size=4, Index=ReferenceSet(b[:].x[:]), ReferenceTo=b[:].x[:] | |||
There was a problem hiding this comment.
Why does the index for this Reference come back as a ReferenceSet instead of {1,2}*{3,4} like the first reference example?
There was a problem hiding this comment.
This is actually something that has not changed in this PR. In this case, the index for m.b[1].x is not the same as the index for m.b[2].x: While those sets are equivalent (both contain [3, 4]), they are not the same set. As a result, it is not proper to make the reference look like it is indexed by the Cartesian product of only two sets. The ReferenceSet is just a notation saying it is defined by the slice iterator (and not simple set operations).
The first example shows the Cartesian product because the block is defined by a Cartesian product. Similarly, there is a variant of this second example that would show the Cartesian product:
>>> from pyomo.environ import *
>>> m = ConcreteModel()
>>> m.I = Set(initialize=[3,4])
>>> @m.Block([1,2])
... def b(b,i):
... b.x = Var(m.I, bounds=(i,None))
...
>>> m.r4 = Reference(m.b[:].x[:])
>>> m.r4.pprint()
r4 : Size=4, Index={1, 2}*I, ReferenceTo=b[:].x[:]
Key : Lower : Value : Upper : Fixed : Stale : Domain
(1, 3) : 1 : None : None : False : True : Reals
(1, 4) : 1 : None : None : False : True : Reals
(2, 3) : 2 : None : None : False : True : Reals
(2, 4) : 2 : None : None : False : True : Reals| """ | ||
| print(buf.getvalue()) | ||
| self.maxDiff = None |
There was a problem hiding this comment.
This is technically only for debugging (unittest truncates long diffs when something like assertEqual fails). Setting maxDiff = None will always show the full diff on failure. I usually take that out when I get the test to stop failing. That said, I have thought about putting that line into the base class defition in pyomo.common.unittest
There was a problem hiding this comment.
I never knew this was an option. I think adding it to the base class would make a lot of sense. This seems like behavior we would always want when tests fail.
Fixes #45, fixes #2068.
Summary/Motivation:
Pyomo creates Set objects on the fly when constructing IndexedComponents indexed by anything other than a single Pyomo Set object. These objects get added to the Block using a fixed auto-generated names. This causes problems when deleting an indexed component and then trying to create it again, because the sets implicitly created by Pyomo are still on the Block and lead to errors about name conflicts.
This PR resolves this by adding support for "anonymous" (unnamed) Set objects. With this PR, he Set objects implicitly created during component declaration (including
index_setanddomain) will not be explicitly attached to the Block. However, they will have theirparent_block()point tot he "owning" block, thereby ensuring that the anonymous sets obey the correct scoping rules when deepcopied / cloned. While this may seem like a departure to the convention that all modeling components are explicitly attached to Blocks, there is precedent in the case of Sets for global Set objects (e.g.,Reals,Integers, etc).This touches a lot of files due to changes in test baselines. If you want to see the diffs for everything except the test changes, see https://github.com/Pyomo/pyomo/compare/d5b53d5..0325346
Changes proposed in this PR:
parent_block()but no_name(use the Set data as the name)_implicit_subsetsattribute (previously used for the implicit arguments to SetProduct) and replaced it with the general purpose_anonymous_sets(used by all anonymous sets)Component.construct()implementationscplex_directin Benders testsLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: