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

replaced seed with key #1167

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Tomas542
Copy link

@Tomas542 Tomas542 commented Jan 6, 2025

Replaced all seed values with key for uniformity of style with jax.random

@Tomas542
Copy link
Author

Tomas542 commented Jan 6, 2025

@rdyro 3 month later, but I've finished it)

@Tomas542
Copy link
Author

Tomas542 commented Jan 6, 2025

Also should close issue #1137 after PR got accepted

@carlosgmartin
Copy link
Contributor

@Tomas542 JAX now recommends using random.key over random.PRNGKey, which will eventually be deprecated. I suggest editing this PR to use the former.

@Tomas542
Copy link
Author

Tomas542 commented Jan 8, 2025

@carlosgmartin yes, I know, but there are some problems with this in tests where NumPy checks for type compatibility. The new keys are of type key<fry> or something like that. It is possible iteratively just replace some of the old styles with the new ones without rewriting the tests if necessary. Tell me, I'll do it

P.S. Accidentally, I've made a mistake in hungarian_algorithm_test, where there were no such problem.

@rdyro rdyro self-requested a review January 8, 2025 19:38
@rdyro
Copy link
Collaborator

rdyro commented Jan 8, 2025

The changes look great so far!

@carlosgmartin is right that we should move to random.key and you're right @Tomas542 that it breaks the current hyperparameter validation funtion in optax.

Let me see if we can fix the optax hyperparameter validation before this PR so that we can stick to random.key

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

Successfully merging this pull request may close these issues.

3 participants