-
Notifications
You must be signed in to change notification settings - Fork 160
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
Speed up orbit computations on integer sets and tuples by providing a better hash function for lists of small integers #5783
Conversation
This seems reasonable, and I agree the old hash function was very bad. A couple of small suggested changes (just because it took me a while to review this, so it might be useful to other people!) These are very picky comments, but I read this fairly carefully because we are hashing bags. Usually Also, I'd make the last argument '1'. I don't think the 'Length(x)' is actually improving the hash value in practice, and I did assume it "meant something", rather than was just a seed. |
Thanks for the comments! I've added a comment explaining that we expect the input to be a list of positive ints as well as an assertion to explicitly check this in the code. To avoid issues with different representations (like if the input were a range), we now also convert the input to a |
Thanks, I'm happy with this now, but I'll let other people pop in and have an opinion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've adjusted the title so it can be used as an entry in the release notes. As such I focused it more on the user facing effect.
This PR improves the hash function used for lists.
Text for release notes
see title
Further details
The existing function essentially determined the hash by onlyconsidering the first two entries of the list. This causes significant lots of hash collision and slowdown when computing the orbit on sets, e.g. before the changes in this PR we get:
After changes: