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

Explicitly define __hash__ for groups #1398

Merged
merged 1 commit into from
Jun 14, 2017
Merged

Conversation

jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented 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

PR Checklist

  • Tests?
  • [ ] Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

Looks good, except cross Universe checks needed

Also needs CHANGELOG entry

b = getattr(u, level_b)[0:-1]
yield _hash_not_equal, a, b


Copy link
Member

Choose a reason for hiding this comment

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

these tests are good, but you also need to check the cross universe case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


u = make_Universe(size=(3, 3, 3))
levels = ('atoms', 'residues', 'segments')
for level_a in levels:
Copy link
Member

Choose a reason for hiding this comment

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

just stylistic, but I'd use itertools here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time today removing itertools use from cython function. It obviously messed up with my brain.

@jbarnoud
Copy link
Contributor Author

I forced pushed an amended commit. I had to fix the reference to the issue in the initial commit message.

@@ -26,7 +26,7 @@ Changes

06/03/17 utkbansal, kain88-de, xiki-tempula, kaplajon, wouterboomsma,
richardjgowers, Shtkddud123, QuantumEntangledAndy, orbeckst,
kaceyreidy
kaceyreidy, jbarnoud
Copy link
Member

Choose a reason for hiding this comment

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

We're in 16.2 now :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jbarnoud jbarnoud force-pushed the issue-1397-group-hash branch 2 times, most recently from e0810d9 to 1717886 Compare June 13, 2017 18:13
@orbeckst
Copy link
Member

Travis full ran into time limit. I restarted it but not sure if it will complete.

@orbeckst orbeckst mentioned this pull request Jun 13, 2017
4 tasks
@orbeckst
Copy link
Member

I have a moment... I will rebase against develop and force push.

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 orbeckst merged commit ebe2b84 into develop Jun 14, 2017
@orbeckst orbeckst deleted the issue-1397-group-hash branch June 14, 2017 00:16
@orbeckst orbeckst mentioned this pull request Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants