-
Notifications
You must be signed in to change notification settings - Fork 53
Add dtype
keyword argument to sum
and prod
#238
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
Conversation
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.
LGTM except for a few nitpicks.
Co-authored-by: Leo Fang <leofang@bnl.gov>
Co-authored-by: Leo Fang <leofang@bnl.gov>
…-api into sum-prod-dtype
…sum-prod-dtype
…sum-prod-dtype
Co-authored-by: Aaron Meurer <asmeurer@gmail.com>
This looks good to me. |
Actually scratch that, I just noticed a potential problem. NumPy's
As written here now, a uint64 array would cast to int64 by default, which would be a downcast (uint64 can represent values that don't fit in the signed int64). |
OK, I think that wording makes it clearer. This does look good now. |
@asmeurer I've updated the text to clarify promotion guidance. |
This has been reviewed. If any further changes are necessary, we can add them in a follow-up PR. |
As the quoted docs say, numpy only applies this to integers (and bool). It does not upcast floats, in particular float32. Is that part intentional, or just a typo? |
@seberg Upcasting floats is intentional, as, in principle (and while not currently required by the spec), also applies to |
@kgryte ah OK. I have not thought much about what I would consider ideal, it is/was just a bit surprising. |
@seberg Were we to make a distinction between integral and floating dtypes for how promotion is handled relative to the default dtypes, this would add complexity to user mental models in terms of when promotion occurs. And further, the same concerns motivating promotion to the default dtype for smaller integral dtypes applies to floating dtypes. Namely, that accumulation can lead to overflow and erroneous results. For example, for When an array library's default floating dtype is |
This PR
adds a
dtype
keyword argument tosum
andprod
based on discussions in gh-202 and in consortium meetings. Adding such a keyword aids in preventing overflows during accumulation.specifies that the default behavior (i.e., with
dtype=None
) be that the returned array have the default data type of the same kind as the data type of the input array, so long as the default data type has a range of values equal to or larger than the input array data type.For example,
int8
array and the default integer data type isint32
, the returned array should have data typeint32
.int64
array and the default integer data type isint32
, the returned array should have data typeint64
.float32
array and the default floating-point data type isfloat64
, the returned array should have data typefloat64
.float64
array and the default floating-point data type isfloat32
, the returned array should have data typefloat64
.In the event a user wants to return an array having the same data type as the input array, a user can explicitly specify the return array
dtype
. The default behavior is intended to prevent potential accumulation footguns.