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

lax.abs: better error for unsigned inputs #17959

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

jakevdp
Copy link
Collaborator

@jakevdp jakevdp commented Oct 5, 2023

Fixes #17958 by explicitly not supporting unsigned inputs to lax.abs.

@jakevdp jakevdp requested a review from hawkinsp October 5, 2023 17:34
@jakevdp jakevdp self-assigned this Oct 5, 2023
@jakevdp jakevdp changed the title lax.abs: support unsigned input lax.abs: better error for unsigned inputs Oct 5, 2023
Copy link
Collaborator

@sharadmv sharadmv left a comment

Choose a reason for hiding this comment

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

Does np.abs handle unsigned ints? I feel like if it does, then our lowering should handle this case and avoid emitting invalid stablehlo and just return its input.

@jakevdp
Copy link
Collaborator Author

jakevdp commented Oct 6, 2023

Both np.abs and jnp.abs handle unsigned ints, the latter by avoiding binding abs_p. lax.abs does not support uints, but the error is a long dump from in mlir. This PR changes the error to something more reasonable.

@sharadmv
Copy link
Collaborator

sharadmv commented Oct 6, 2023

I guess I'm asking if it should handle it? Pallas users might call lax.abs and we'd want to lower that to Mosaic.

The MLIR lowering can raise an error instead of the lax.abs function itself to avoid the stablehlo error.

@jakevdp
Copy link
Collaborator Author

jakevdp commented Oct 6, 2023

In most cases, we define dtype rules for lax primitives so that dtype-related errors are caught during abstract evaluation, rather than letting those errors arise from a lower level. So I think this change is consistent with overall design decisions we've made in the rest of lax. What do you think?

@google-ml-butler google-ml-butler bot added kokoro:force-run pull ready Ready for copybara import and testing labels Oct 6, 2023
@copybara-service copybara-service bot merged commit fae53d9 into jax-ml:main Oct 6, 2023
@jakevdp jakevdp deleted the lax-abs branch October 6, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lax.abs crashes on unsigned ints
4 participants