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

fix: return ufunc as it is for user defined vectorized funcs #3025

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

Saransh-cpp
Copy link
Member

Fixes #2603

@Saransh-cpp Saransh-cpp marked this pull request as draft February 15, 2024 13:20
@Saransh-cpp Saransh-cpp force-pushed the custom_behaviors_with_jax branch from 0ad9a14 to 9b39852 Compare February 15, 2024 13:32
@Saransh-cpp Saransh-cpp changed the title fix: don't use jax.numpy while wrapping vectorized funcs as ufuncs fix: return ufunc as it is for user defined vectorized funcs Feb 15, 2024
@Saransh-cpp Saransh-cpp marked this pull request as ready for review February 15, 2024 13:32
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.94%. Comparing base (b749e49) to head (0c79601).
Report is 30 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
src/awkward/_connect/jax/__init__.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@agoose77
Copy link
Collaborator

@Saransh-cpp this looks good to me! I anticipate problems if the custom ufunc doesn't understand JAX arrays, but I suppose for now we can try the ufunc to see what happens. In future, we might want some kind of mechanism to associate e.g. a numba ufunc with a JAX-aware overload, but that can be a later step.

@Saransh-cpp
Copy link
Member Author

I anticipate problems if the custom ufunc doesn't understand JAX arrays

I see, this makes sense. Looking at this again, shouldn't this be on the user side? If I understand correctly, shouldn't a user ensure that their functions are compatible with JAX arrays if they are planning to use the jax backend?

@agoose77
Copy link
Collaborator

I anticipate problems if the custom ufunc doesn't understand JAX arrays

I see, this makes sense. Looking at this again, shouldn't this be on the user side? If I understand correctly, shouldn't a user ensure that their functions are compatible with JAX arrays if they are planning to use the jax backend?

Yes I think so. As it stands, I'm not sure what that mechanism would be. It would be nice if a user can leverage someone-else's code, and patch their ufuncs to make JAX work, for example.

@Saransh-cpp
Copy link
Member Author

Yes I think so. As it stands, I'm not sure what that mechanism would be. It would be nice if a user can leverage someone-else's code, and patch their ufuncs to make JAX work, for example.

Oh I see, yes that would be nice too. I can look into this if any issues pop up in the future. Thanks!

@jpivarski
Copy link
Member

What's the status of this one?

@Saransh-cpp
Copy link
Member Author

The fix works! Jax and numba might still clash but that would be on the user side and not on the awkward side. But as mentioned by @agoose77, I'll look into providing users with an awkward-fix (maybe a decorator?) for their functions if we get any complaints.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Okay, great! And it's such a simple change on the Awkward side, with a test, that I'll just merge it now. Thanks!

@jpivarski jpivarski enabled auto-merge (squash) March 5, 2024 15:51
@jpivarski jpivarski merged commit 65f1ea7 into scikit-hep:main Mar 5, 2024
39 checks passed
@Saransh-cpp Saransh-cpp deleted the custom_behaviors_with_jax branch March 5, 2024 17:57
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.

Custom behaviors plus jax leading to lookup in wrong spot
3 participants