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

Some objects that define __eq__ should define __hash__ #843

Closed
adler-j opened this issue Jan 20, 2017 · 6 comments
Closed

Some objects that define __eq__ should define __hash__ #843

adler-j opened this issue Jan 20, 2017 · 6 comments

Comments

@adler-j
Copy link
Member

adler-j commented Jan 20, 2017

Basically the standard (since python 3) says:

If a class does not define an __eq__() method it should not define a __hash__() operation either; if it defines __eq__() but not __hash__(), its instances will not be usable as items in hashable collections. If a class defines mutable objects and implements an __eq__() method, it should not implement __hash__(), since the implementation of hashable collections requires that a key’s hash value is immutable (if the object’s hash value changes, it will be in the wrong hash bucket).

So, I noticed this while trying to hash ODL spaces in #840.

Luckily, all of our spaces are immutable, as are all other sets. Hence we should add __hash__() to them.

I'll solve this as part of #840.

@kohr-h
Copy link
Member

kohr-h commented Jan 20, 2017

Yes, I think common practice is to use the "defining" members of a class and XOR their hashes.

Related topic for later: __getstate__ and __setstate__ are also interesting, especially when sending stuff over the wire. The standard way to pickle is to pickle an object's __dict__, which can be wasteful, for example for uniform grids (after merging #841).

@kohr-h
Copy link
Member

kohr-h commented Jan 20, 2017

Second thought on hashing: I'm not so sure if it makes sense for spaces. There are a bunch of numerical values involved, most prominently exponent and weighting (usually a constant), and here we're really sensitive to rounding. So perhaps we should add the method only to the most basic objects that don't involve any floating point numbers.

@adler-j
Copy link
Member Author

adler-j commented Jan 20, 2017

Well we simply have to ensure that any two spaces who compare equal also hash equal, I think we don't have any fancy "close" tests for exponents etc, but actually require strict equality, no?

@adler-j
Copy link
Member Author

adler-j commented Jan 20, 2017

Did a very simply addition of simply hashing all things that went into the __eq__ method, which should be the "correct" method.

@kohr-h
Copy link
Member

kohr-h commented Jan 21, 2017

Interesting, I didn't know that sets in Py3 use hashing to determine duplicates. Makes sense though, and makes me wonder what Py2 used to do. Probably __eq__?

@kohr-h
Copy link
Member

kohr-h commented Jan 21, 2017

Well we simply have to ensure that any two spaces who compare equal also hash equal, I think we don't have any fancy "close" tests for exponents etc, but actually require strict equality, no?

Sure, and I was more imagining somebody trying to use a space as a dictionary key. And whenever any of the space properties rely on computations, we're doomed (regarding hashing). But it's not a big deal I guess, __eq__ will also return False then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants