Overhaul the Suffix component
#3072
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3072 +/- ##
==========================================
+ Coverage 88.26% 88.29% +0.02%
==========================================
Files 832 832
Lines 92301 92243 -58
==========================================
- Hits 81474 81447 -27
+ Misses 10827 10796 -31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| args = [ | ||
| arg | ||
| for arg in (kwargs.pop(name, notset) for name in namelist) | ||
| if arg is not notset |
There was a problem hiding this comment.
Same question as above: !=?
There was a problem hiding this comment.
Here we do not want to look for equivalence (==) but identity (is): we are looking to collect all arguments that the user specified. This is almost equivalent to
args = []
for key in kwargs:
if key not in kwargs:
continue
arg = kwargs[key]
if arg is not notset:
args.append(arg)dict.get(key, notset) is not notset is a shorthand for testing __contains__ before fetching the value.
Additionally, besides performance (is is faster than ==), the thing we worry about is the values can be Components, and we override __eq__ for a lot of components to perform expression generation -- which we don't want to trigger here.
| yield name, suffix | ||
| def suffix_generator(a_block, datatype=NOTSET, direction=NOTSET, active=None): | ||
| _iter = a_block.component_map(Suffix, active=active).items() | ||
| if direction is not NOTSET: |
There was a problem hiding this comment.
Same question as above: !=?
There was a problem hiding this comment.
Here we are testing if the optional argument direction was provided by the user -- that is, is it ANYTHING other than the default value from the function specification. is is faster, more appropriate, and avoids possible side effects due to __eq__ overrides.
| return set((type(val), id(val)) for val in self) == set( | ||
| (type(val), id(val)) for val in other | ||
| ) | ||
| return all(id(key) in self._data for key in other) |
There was a problem hiding this comment.
What about keys in self but not other?
There was a problem hiding this comment.
That is covered by the test above (if len(self) != len(other)): if their lengths are the same, then you only have to test one side (because neither can have duplicates). I will break that test apart to make it a little more clear
Fixes # .
Summary/Motivation:
Recent development (in parmest / DoE) highlighted that the set of admissible inputs to
Suffix(initialize=...)was more restrictive than the rest of Pyomo. This moves Suffix to use the standardInitializerframework (for consistency / flexibility).As part of this work, this PR does a general update / refresh of the entire component:
AbstractSuffixclass to prevent using the Suffix before it has been constructedAnd some (necessary) general improvements:
_pop_from_kwargsfromIndexedComponenttoComponentComponentMap.__eq__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: