Skip to content

Explicitly specify which categories of dtypes the elementwise functions are defined for #121

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 6 commits into from
Feb 21, 2021

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Jan 30, 2021

Please double check that I didn't make any mistakes.

Some comments:

  • I made all transcendental functions require floating-point inputs (i.e., no integers). This is because they say the output must use type promotion, but integer dtypes do not type promote to floating-point dtypes according to the type promotion rules.
  • For pow, pow(a, b) does not make sense if a and b are integers and b is negative. Should we restrict it to only being defined for nonnegative b? I think I might have brought this up before somewhere, but I don't remember what was decided.
    • (as an aside, shouldn't pow(0, 0) == 1 be listed as a special case for pow?)
  • The same question for divide. Should it be defined for integer inputs, and if so, does that preclude the definition when the second argument is 0?

I will also update the array methods as per #98 (comment) in another PR once this is merged.

@rgommers
Copy link
Member

Thanks Aaron. This categorization makes sense, and I agree with "must" -> "should".

@rgommers
Copy link
Member

I made all transcendental functions require floating-point inputs (i.e., no integers). This is because they say the output must use type promotion, but integer dtypes do not type promote to floating-point dtypes according to the type promotion rules.

This seems correct to me. Most libraries will happily accept integer dtypes and upcast to float, but IIRC at least TensorFlow raises an exception instead.

+1 for the pow proposal and listing the special case.

divide should not be limited I believe. It's different from transcendental functions, because it's more important that it matches / (which in Python 3 does int / int -> float) than that there's no casting rules for int to float.

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.

Caught two typos while digesting the changes 🙂

@kgryte
Copy link
Contributor

kgryte commented Feb 1, 2021

The special case of pow(0,0) is already addressed in the spec by:

If x2_i is +0, the result is 1, even if x1_i is NaN.
If x2_i is -0, the result is 1, even if x1_i is NaN.

@kgryte
Copy link
Contributor

kgryte commented Feb 1, 2021

Re: divide. If integer dtypes are to be explicitly permitted in the spec, we should state explicitly something to the effect of

If x1 has an integer dtype, the array must be cast to the default floating-point data type prior to computing results.

Ditto for x2.

If the goal is to match Python 3 / behavior exactly, then we'd need to recommend casting to float64, but this does not seem desirable.

@asmeurer
Copy link
Member Author

asmeurer commented Feb 1, 2021

I made pow and divide only defined for floating-point inputs.

Co-authored-by: Athan <kgryte@gmail.com>
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now. Thanks @asmeurer, and thanks @kgryte and @leofang for reviewing.

@rgommers rgommers merged commit 2e4520a into main Feb 21, 2021
@rgommers rgommers deleted the elementwise-data-type-categories branch February 21, 2021 15:23
@rgommers
Copy link
Member

I will also update the array methods as per #98 (comment) in another PR once this is merged.

Can you do this follow-up @asmeurer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants