Optimize standard gate library equality #11370
Conversation
|
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.
256b747 to
a993f95
Compare
Pull Request Test Coverage Report for Build 7618275801
💛 - Coveralls |
| @@ -959,6 +980,9 @@ def inverse(self): | |||
| """Invert this gate. The C4X is its own inverse.""" | |||
| return C4XGate(ctrl_state=self.ctrl_state) | |||
|
|
|||
| def __eq__(self, other): | |||
| 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.
| @@ -112,3 +112,6 @@ def _define(self): | |||
| def inverse(self): | |||
| """Return inverse ECR gate (itself).""" | |||
| 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.
| @@ -222,3 +225,6 @@ def _define(self): | |||
| def inverse(self): | |||
| """Return inverted CH gate (itself).""" | |||
| 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?
| 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()?
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.
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: