-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Refactored common upcast for integral-type accumulators #20842
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
Refactored common upcast for integral-type accumulators #20842
Conversation
16b7a14 to
52e08a3
Compare
1b23953 to
a7255f7
Compare
d6f2e04 to
a0e30af
Compare
a0e30af to
93cda81
Compare
|
@jakevdp should be ready for review |
93cda81 to
e00f7df
Compare
e00f7df to
23f146e
Compare
23f146e to
6b249f0
Compare
6b249f0 to
09c4242
Compare
09c4242 to
75758b2
Compare
jakevdp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
|
Could you sync against the updated main branch and resolve conflicts? Thanks! |
75758b2 to
cd288ee
Compare
|
Synced! |
|
It looks like this breaks some tests in It looks like this somehow changed the behavior for integer inputs narrower than int32. |
561bb6c to
e115797
Compare
|
It looks like the core issue is that we don't allow
Update: I've set it to keep |
ce9771e to
e5c093d
Compare
|
@jakevdp should be good for another look now |
jakevdp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this PR makes ``promote_integers` a public keyword for all cumulative reductions. Do we need to let users see that? Could we do this in a way that doesn't affect the public-facing API?
e5c093d to
b22b109
Compare
|
Good point. I've changed it so that we make a helper reduction |
b22b109 to
34c5163
Compare
jakevdp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Towards #20200
This PR refactors integer accumulator promotion from
jax._src.numpy.reductions._reductionand applies it to_make_cumulative_reductionas well.Temporarily disables tests for
prod,sum, andtracesince the 2023 API included breaking changes which are not yet accounted for in the tests repository.