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

seed Dict so that hashes get non-zero h #57277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented Feb 5, 2025

This makes it so that we call the 2 arg hash from Dict so that hashing functions can always rely on having entropy in h to help mix things up. This will obviously need testing and pkgeval to make sure it doesn't break any user expectations(which it really shouldn't)

@giordano giordano added needs tests Unit tests are required for this change needs pkgeval Tests for all registered packages should be run with this change labels Feb 5, 2025
@JeffBezanson
Copy link
Member

Why exactly is this needed? It's better to avoid advancing the random stream in various places, and it could also be quite surprising that every dict will put the same keys in a different order. Yes, you're not supposed to depend on the order, but it's surprising/weird.

@Keno
Copy link
Member

Keno commented Feb 6, 2025

Yeah, not a fan of having Dict allocation become effectful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashing needs pkgeval Tests for all registered packages should be run with this change needs tests Unit tests are required for this change performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants