Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make EqualsTester notice incorrect delegation #3809

Merged
merged 11 commits into from
Mar 25, 2021

Conversation

dabacon
Copy link
Collaborator

@dabacon dabacon commented Feb 16, 2021

This extends EqualsTester to notice when someone incorrectly does not return NotImplemented in __eq__ for objects of an unrecognized type. Doing this can cause commutativity to fail when implementing other objects that should be equal to this object. (It was worse in python 2, where depending on whether one inherited from object or not could cause this).

This also fixes places where Cirq code was doing this incorrectly.

Fixes #877

One final note is that the eigen_gate_test was changed because apparently Sympy does this incorrectly

>>> import sympy
>>> s = sympy.Symbol("a")
>>> class MyClass:
...   def __eq__(self, other):
...     return True
... 
>>> s.__eq__(MyClass())
False

@dabacon dabacon requested review from cduck, vtomole, wcourtney and a team as code owners February 16, 2021 02:23
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Feb 16, 2021
@dabacon
Copy link
Collaborator Author

dabacon commented Feb 16, 2021

Cirq is secretly just a test of places where Sympy fails to be pythonic?

@balopat
Copy link
Contributor

balopat commented Feb 16, 2021

I just reran your checks as well on this PR to see the difference, couldn't repro the lint error yet on this branch. And now it passed. Very odd. XRef #3716.

@dabacon
Copy link
Collaborator Author

dabacon commented Feb 17, 2021

@Strilanc probably the best one to look at this sense he wrote the original definition of the problem (and there seems to be a slight difference in what he said, since we don't need a fallback to reference checking to get this work).

@dabacon
Copy link
Collaborator Author

dabacon commented Mar 24, 2021

@balopat this could use a triage to a reviewer

Copy link
Collaborator

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments in the test here are incredibly helpful for parsing it, thanks!

One nit and this is good to go.

cirq/ops/eigen_gate_test.py Outdated Show resolved Hide resolved
@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 24, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 24, 2021
@dabacon
Copy link
Collaborator Author

dabacon commented Mar 24, 2021

tx @95-martin-orion !

@CirqBot CirqBot merged commit 8fb3efe into quantumlib:master Mar 25, 2021
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 25, 2021
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 25, 2021
@dabacon dabacon deleted the delequals branch April 16, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add delegated equality test to EqualsTester
5 participants