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

Don't serialize GridQubit hash when pickling #3791

Merged
merged 5 commits into from Feb 17, 2021
Merged

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Feb 12, 2021

When hashing Qid objects we were including the Qid class, but class hashes are not stable between restarts of the interpreter. This caused #3777 where the hash of unpickled GridQubit objects disagrees with other GridQubits to which they compare equal. Here we fix this by hashing based on the class name rather than the class object itself.

Fixes #3777

@maffoo maffoo requested review from cduck, vtomole and a team as code owners February 12, 2021 00:08
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Feb 12, 2021
@mpharrigan
Copy link
Collaborator

Are string hashes stable :)

@maffoo
Copy link
Contributor Author

maffoo commented Feb 12, 2021

@mpharrigan, that is a very good question :-)

I also noticed the tests were failing because we specifically do not want the hashes to depend on the particular Qid class, since, e.g. GridQubit(0, 0) and GridQid(0, 0, dimension=2) compare equal so should hash to the same thing. I've switched instead to just implementing the __getstate__ and __setstate__ functions used when pickling to skip storing the hash and recompute it when unpickling. Also added a test that this works as intended with pickle.

@dabacon
Copy link
Collaborator

dabacon commented Feb 13, 2021

Looks like there are also other places where the hash of the class is used, like Moment?

@maffoo
Copy link
Contributor Author

maffoo commented Feb 17, 2021

I think hash of a class is fine; we just have to avoid serializing hashes when e.g. pickling, because the hashes are not stable from one run of the python interpreter to the next. I'll change the issue title to reflect this.

@maffoo maffoo changed the title Don't use Qid class object when hashing Don't serialize GridQubit hash when pickling Feb 17, 2021
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

awesome! Thanks for doing the fix

@maffoo maffoo added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 17, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 17, 2021
@maffoo maffoo removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 17, 2021
@maffoo maffoo merged commit e46f62e into master Feb 17, 2021
@maffoo maffoo deleted the u/maffoo/gridqubit-hash branch February 17, 2021 22:34
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.

q1 == q1 but hash(q1) != hash(q2)
5 participants