Skip to content

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

Merged
merged 13 commits into from
Sep 17, 2021
Merged

Add dtype keyword argument to sum and prod #238

merged 13 commits into from
Sep 17, 2021

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Jul 26, 2021

This PR

  • adds a dtype keyword argument to sum and prod 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,

    • if provided an int8 array and the default integer data type is int32, the returned array should have data type int32.
    • if provided an int64 array and the default integer data type is int32, the returned array should have data type int64.
    • if provided a float32 array and the default floating-point data type is float64, the returned array should have data type float64.
    • if provided a float64 array and the default floating-point data type is float32, the returned array should have data type float64.

    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.

@kgryte kgryte added the API change Changes to existing functions or objects in the API. label Jul 26, 2021
Copy link
Contributor

@leofang leofang left a 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: Aaron Meurer <asmeurer@gmail.com>
@asmeurer
Copy link
Member

This looks good to me.

@asmeurer
Copy link
Member

Actually scratch that, I just noticed a potential problem. NumPy's dtype argument to sum is documented as follows:

dtype : dtype, optional
   The type of the returned array and of the accumulator in which the
   elements are summed.  The dtype of `a` is used by default unless `a`
   has an integer dtype of less precision than the default platform
   integer.  In that case, if `a` is signed then the platform integer
   is used while if `a` is unsigned then an unsigned integer of the
   same precision as the platform integer is used.

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).

@asmeurer
Copy link
Member

OK, I think that wording makes it clearer. This does look good now.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 17, 2021

@asmeurer I've updated the text to clarify promotion guidance.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 17, 2021

This has been reviewed. If any further changes are necessary, we can add them in a follow-up PR.

@seberg
Copy link
Contributor

seberg commented Sep 17, 2021

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?

@kgryte kgryte merged commit 25e24ca into main Sep 17, 2021
@kgryte kgryte deleted the sum-prod-dtype branch September 17, 2021 23:14
@kgryte
Copy link
Contributor Author

kgryte commented Sep 17, 2021

@seberg Upcasting floats is intentional, as, in principle (and while not currently required by the spec), also applies to float16 (bfloat16) in accelerator libraries, where overflow/precision concerns are still valid, albeit less of common concern than, e.g., int8.

@seberg
Copy link
Contributor

seberg commented Sep 17, 2021

@kgryte ah OK. I have not thought much about what I would consider ideal, it is/was just a bit surprising.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 17, 2021

@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 float32, the maximum safe integer is less than 2e7, leading to potential user footguns when summing and/or multiplying many large positive numbers. If the default floating dtype for a specification-compliant array library is float64 and dtype is None, the library should promote to float64 to avoid overflow and/or precision errors.

When an array library's default floating dtype is float64, if a user wants to return a float32 array when providing a float32 array, they can explicitly specify via the dtype kwarg that they wish to receive an output array of the same dtype. (This same logic applies to a user who provides an int8 array and wants an int8 array in return). Whether an array library chooses to accumulate internally in float32 is, however, an implementation detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants