Optimize standard gate library equality#11370
Conversation
mtreinish
added
performance
Changelog: None
|
One or more of the the following people are requested to review this:
|
This commit updates the standard library gate classes' to define an __eq__ methods to optimize the runtime performance. Their parent class `Instruction`'s __eq__ is optimized for generality to compare equality between any gate instruction object. But this generality has a runtime cost which is unnecessary for the standard library gates which it equality is much simpler. This commit adds the faster path equality check for the standard gate library classes which will improve the performance of anything doing a lot gate equality.
mtreinish
force-pushed
the
standard-lib-eq-update
branch
from
256b747 to
a993f95
Compare
Pull Request Test Coverage Report for Build 7618275801
💛 - Coveralls |
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
Thanks for this - I think this'll help performance of quite a lot of little bits and bobs across the code base.
| return isinstance(other, C4XGate) and self.ctrl_state == other.ctrl_state | ||
|
|
||
|
|
||
| class MCXGate(ControlledGate): |
There was a problem hiding this comment.
This might well want to be a follow-up, but if we could get a decent way to improving the MCXGate equality checks, it'd help (part of) #10641.
| return ECRGate() # self-inverse | ||
|
|
||
| def __eq__(self, other): | ||
| return isinstance(other, ECRGate) |
There was a problem hiding this comment.
If someone were to extend ECRGate via inheritance, this definition of __eq__ would be wrong for that subclass. Is it worth instead defining this once for SingletonGate as:
def __eq__(self, other):
return isinstance(other, type(self))There was a problem hiding this comment.
That wouldn't actually be correct for singleton gates, where the singleton instance is a separate part of the inheritance tree to the non-singleton versions.
I'm not super convinced that __eq__ should attempt to drive additional logic within subclasses; we can't really know what additional information a subclass might add on, nor what should really cause them to be considered equal. It largely seems to me that subclasses should almost always be responsible for overriding __eq__.
There was a problem hiding this comment.
Ah, I forgot about that nuance with the class hierarchy. It seems to me like __eq__ should behave the same for all SingletonGates. If there is more state to compare, then I'd think it's no longer a SingletonGate from a traiting perspective.
| return CHGate(ctrl_state=self.ctrl_state) # self-inverse | ||
|
|
||
| def __eq__(self, other): | ||
| return isinstance(other, CHGate) and self.ctrl_state == other.ctrl_state |
There was a problem hiding this comment.
Same comment. Perhaps a shared implementation is possible for all SingletonControlledGate instances?
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
This looks fine to me, but I'm not tagging for merge so Kevin can respond as well.
| def _compare_parameters(self, other): | ||
| for x, y in zip(self.params, other.params): | ||
| try: | ||
| if not math.isclose(x, y, rel_tol=0, abs_tol=1e-10): | ||
| return False | ||
| except TypeError: | ||
| if x != y: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
I guess specifically this is stdlib-gate parameters, but that's fine.
There was a problem hiding this comment.
Yeah it's why I made it private, I can add a comment or docstring for it. Or were you propoposing I rename it _compare_stdlib_gate_parameters()?
kevinhartman
left a comment
kevinhartman
left a comment
There was a problem hiding this comment.
Looks good to me.
I see now why it'd be difficult to define __eq__ for some base in the class hierarchy.
Most standard-library gates had a specialised `__eq__` added in Qiskitgh-11370, but a small number were left over. This adds specialisation to almost all the stragglers, which were mostly old soft-deperecated gates. Despite not appearing in most circuits, these gates have an outsized effect on equality comparisons if they ever _do_ appear, because they involve construction of entire `definition` fields.
…13327) Most standard-library gates had a specialised `__eq__` added in gh-11370, but a small number were left over. This adds specialisation to almost all the stragglers, which were mostly old soft-deperecated gates. Despite not appearing in most circuits, these gates have an outsized effect on equality comparisons if they ever _do_ appear, because they involve construction of entire `definition` fields.
5 participants
Summary
This commit updates the standard library gate classes' to define an eq methods to optimize the runtime performance. Their parent class
Instruction's eq is optimized for generality to compare equality between any gate instruction object. But this generality has a runtime cost which is unnecessary for the standard library gates which it equality is much simpler. This commit adds the faster path equality check for the standard gate library classes which will improve the performance of anything doing a lot gate equality.Details and comments
TODO: