feat: add axis=None specialization for Min reducer + small refactoring for existing specializatons#3896
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3896 |
axis=None specialization for Min reducer + delete jax backend specializationsaxis=None specialization for Min reducer + testing for all specialization axes + delete jax backend specializations
axis=None specialization for Min reducer + testing for all specialization axes + delete jax backend specializationsaxis=None specialization for Min reducer + testing for all specializations + delete jax backend specializations
|
@ianna if you could please check that the same speedup is there for |
ianna
left a comment
There was a problem hiding this comment.
@ikrommyd - please keep the JAX code in - we need it to run the test in preparation for the next week blueprint. As for the other changes - they are not random and have been tested on the previous reducer and did contribute to the performance gains. Thanks.
|
I can keep the jax code in and also add the an axis none min reducer for jax too, but I don't understand how it has anything to do with the blueprint. jax backend is slow, unjittable, and deprecated. How does this performance optimization help? Reductions work for jax, they just do not use this optimization. Also with the previous code, they were not being used for jax anyways because they jax reducers do not implement the |
Thanks! I will be taking about JAX and want to cross the tees — just to double check performance statements we made in the past. |
Oh, missed that part about implementing extra reducers for JAX — I don’t think it’s necessary at this point. Thanks |
|
I'm not following perfectly. In the last PR, you attempted adding a fast-path axis none specialization for sum and max for jax? Do you want those in or not? |
Yes, I want |
|
Then we'll have |
I completely agree that |
Ah I see, okay then I would suggest you do this locally or on another remote branch by just deleting the |
axis=None specialization for Min reducer + testing for all specializations + delete jax backend specializationsaxis=None specialization for Min reducer + small refactoring for existing specializatons
|
@ianna removed the tests as those code paths were being hit anyways. Is this good to go now? |
This adds
axis=Nonefast path specialization forMin.We also do some cleanup at the same time and remove the existing sum/max specializations for jax.
(I am sneaking in one more deprecation warning here for jax so that it's also raised when one does
ak.jax.register_and_check()).