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

AtomGroups are not hashable on python 3 #1397

Closed
jbarnoud opened this issue Jun 13, 2017 · 3 comments
Closed

AtomGroups are not hashable on python 3 #1397

jbarnoud opened this issue Jun 13, 2017 · 3 comments

Comments

@jbarnoud
Copy link
Contributor

Expected behaviour

An AtomGroup is hashable, it can therefore be placed in a set or as a dictionary key. topologyattr.Bonds.fragments, that creates a set of AtomGroups, works as intended.

Actual behaviour

Attempting to get the hash of an AtomGroup on python 3 raises the following error:

TypeError: unhashable type: 'AtomGroup'

It works on python 2.

In python 3, a class that defines __eq__ but not __hash__ has __hash__ set to None, see https://docs.python.org/3/reference/datamodel.html#object.__hash__. So we should define a __hash__ method for the GroupBase class. I think this method could look like:

def __hash__(self):
    return hash((self.universe, self.__class__, self._ix))

Code to reproduce the behaviour

#!/usr/bin/env python

import MDAnalysis as mda
from MDAnalysisTests.datafiles import TPR, XTC

u = mda.Universe(TPR, XTC)

print(hash(u.atoms))

Currently version of MDAnalysis:

(run python -c "import MDAnalysis as mda; print(mda.__version__)")
'0.16.2-dev0' (7652710)

@richardjgowers
Copy link
Member

Looks good.

self.__class__ already implicitly contains self.universe because of the metaprogramming we do (I think?), but it's harmless and more explicit to also put universe into the hash.

We shouldn't use self._ix as the underscore bypasses any magic we want to put into the property, eg UpdatingAtomGroup wouldn't update itself when ._ix is accessed.

@richardjgowers
Copy link
Member

@jbarnoud if you want, you can add a knownfailure test and merge that, then @tylerjereddy or I will make sure that this passes for py3

@jbarnoud
Copy link
Contributor Author

@richardjgowers I have a PR almost ready.

jbarnoud added a commit that referenced this issue Jun 13, 2017
Groups (AtomGroup, ResidueGroup, SegmentGroup) cannot be stored in sets
or used as dict key if they are not hashable. In python 3, the __hash__
method is not defined implicitly anymore when a class has a __eq__ method.

Fixes #1397
jbarnoud added a commit that referenced this issue Jun 13, 2017
Groups (AtomGroup, ResidueGroup, SegmentGroup) cannot be stored in sets
or used as dict key if they are not hashable. In python 3, the __hash__
method is not defined implicitly anymore when a class has a __eq__ method.

Fixes #1397
jbarnoud added a commit that referenced this issue Jun 13, 2017
Groups (AtomGroup, ResidueGroup, SegmentGroup) cannot be stored in sets
or used as dict key if they are not hashable. In python 3, the __hash__
method is not defined implicitly anymore when a class has a __eq__ method.

Fixes #1397
orbeckst pushed a commit that referenced this issue Jun 14, 2017
Groups (AtomGroup, ResidueGroup, SegmentGroup) cannot be stored in sets
or used as dict key if they are not hashable. In python 3, the __hash__
method is not defined implicitly anymore when a class has a __eq__ method.

Fixes #1397
orbeckst added a commit that referenced this issue Jun 14, 2017
- Explicitly define __hash__ for groups:
   Groups (AtomGroup, ResidueGroup, SegmentGroup) cannot be stored in sets
   or used as dict key if they are not hashable. In python 3, the hash
   method is not defined implicitly anymore when a class has a eq method.
- fixes #1397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants