-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
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
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 |
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
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._reduction
and applies it to_make_cumulative_reduction
as well.Temporarily disables tests for
prod
,sum
, andtrace
since the 2023 API included breaking changes which are not yet accounted for in the tests repository.