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

Generalizes safe_int32_increment to safe_increment #1054

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

copybara-service[bot]
Copy link

Generalizes safe_int32_increment to safe_increment

@copybara-service copybara-service bot force-pushed the test_673538782 branch 5 times, most recently from ff41391 to 6c367f4 Compare September 12, 2024 17:32
@copybara-service copybara-service bot merged commit c0e4228 into main Sep 12, 2024
@copybara-service copybara-service bot deleted the test_673538782 branch September 12, 2024 17:51
@carlosgmartin
Copy link
Contributor

carlosgmartin commented Oct 6, 2024

@vroulet I see that optax.safe_increment rejects unsigned integers, and its code contains the following rationale as a comment:

The comparison count < max_value appears to convert its arguments into signed integers so we get overflow errors with unsigned integers.

It looks like performing the comparison against an array containing max_value with the dtype of count solves this problem:

>>> from jax import numpy as jnp
>>> count = jnp.array(0, jnp.uint32)
>>> max_val = jnp.iinfo(count.dtype).max
>>> count < max_val
OverflowError: Python int 4294967295 too large to convert to int32
>>> count < jnp.array(max_val, count.dtype)
Array(True, dtype=bool)

If that's a valid solution, I can submit a PR for this.

@vroulet
Copy link
Collaborator

vroulet commented Oct 6, 2024

Ah I should have thought about this.
You can send a PR to fix this (add corresponding tests). Thanks for catching this!

@carlosgmartin
Copy link
Contributor

@vroulet Done: #1092.

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.

2 participants