Allow construction of CUID from another CUID#3464
Conversation
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
Some implementation questions.
pyomo/core/base/componentuid.py
Outdated
| except (OSError, IOError): | ||
| self._cids = tuple(self._parse_cuid_v1(component)) | ||
|
|
||
| elif isinstance(component, ComponentUID): |
There was a problem hiding this comment.
In find_component, we used type is ComponentUID instead of isinstance, presumably for performance. Should we do the same here?
There was a problem hiding this comment.
Probably, yes. (If for no other reason than consistency - a derived class would work here, but not in find_component)
pyomo/core/base/componentuid.py
Outdated
| elif isinstance(component, ComponentUID): | ||
| if context is not None: | ||
| _context_err(ComponentUID) | ||
| self._cids = copy.deepcopy(component._cids) |
There was a problem hiding this comment.
IIRC, _cids shouldn't contain anything mutable, so we may not need the copy here.
There was a problem hiding this comment.
I believe that is correct, and would be in favor of the change.
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
A couple super minor changes that would be nice to include and this should be good to go.
pyomo/core/base/componentuid.py
Outdated
| def _context_err(_type): | ||
| raise ValueError( | ||
| f"Context is not allowed when initializing a ComponentUID from {_type}." | ||
| ) | ||
|
|
There was a problem hiding this comment.
We should probably move this up to the module scope so we don't take the hit of re-creating the function at every call ti __init__
pyomo/core/base/componentuid.py
Outdated
| except (OSError, IOError): | ||
| self._cids = tuple(self._parse_cuid_v1(component)) | ||
|
|
||
| elif isinstance(component, ComponentUID): |
There was a problem hiding this comment.
Probably, yes. (If for no other reason than consistency - a derived class would work here, but not in find_component)
pyomo/core/base/componentuid.py
Outdated
| elif isinstance(component, ComponentUID): | ||
| if context is not None: | ||
| _context_err(ComponentUID) | ||
| self._cids = copy.deepcopy(component._cids) |
There was a problem hiding this comment.
I believe that is correct, and would be in favor of the change.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3464 +/- ##
==========================================
- Coverage 88.59% 88.18% -0.41%
==========================================
Files 880 880
Lines 100555 100556 +1
==========================================
- Hits 89083 88672 -411
- Misses 11472 11884 +412
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Approved, pending successful tests.
4 participants
Fixes
ComponentUID(ComponentUID("x[1]"))raises an error.Summary/Motivation:
When processing keys corresponding to components, e.g., in
find_componentorcontrib.mpc.get_indexed_cuid, we sometimes want to convert a key into a CUID so it can be handled more consistently. However, we have to handle separately the case where the key is already a CUID. This PR proposes to allow construction of a CUID from another CUID.Changes proposed in this PR:
ComponentUIDis anotherComponentUID, we just copy the_cidsattribute onto the new instance.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: