Skip to content
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

BUG: Incorrect type list for some ufunc loops (sin, cos, arctan2) #22984

Closed
seberg opened this issue Jan 10, 2023 · 2 comments · Fixed by #22986
Closed

BUG: Incorrect type list for some ufunc loops (sin, cos, arctan2) #22984

seberg opened this issue Jan 10, 2023 · 2 comments · Fixed by #22986

Comments

@seberg
Copy link
Member

seberg commented Jan 10, 2023

This is a follow up from gh-22376, where I forgot the sin/cos loops. CuPy noticed these in cupy/cupy#7243 and also found that arctan2 has the same issue.

For arctan2 this is actually a bug also leads to wrong results within NumPy (rather than only causing numba/cupy to be confused).

Just opening the issue in case 1.24.2 is around the corner, should fix it today in either case.

@seberg seberg added this to the 1.24.2 release milestone Jan 10, 2023
@kmaehashi
Copy link
Member

Thanks for opening an issue @seberg! It seems sin and cos are also affected when int8 is involved because np.can_cast(np.int8, np.float16) is True:

NumPy 1.23:

>>> np.sin.types
['e->e', 'f->f', 'd->d', 'f->f', 'd->d', 'g->g', 'F->F', 'D->D', 'G->G', 'O->O']
>>> np.sin(np.array([0, 1, 2], dtype=np.int8))
array([0.    , 0.8413, 0.909 ], dtype=float16)
>>> np.cos(np.array([0, 1, 2], dtype=np.int8))
array([ 1.    ,  0.5405, -0.4163], dtype=float16)

NumPy 1.24:

>>> np.sin.types
['f->f', 'e->e', 'd->d', 'f->f', 'd->d', 'g->g', 'F->F', 'D->D', 'G->G', 'O->O']
>>> np.sin(np.array([0, 1, 2], dtype=np.int8))
array([0.       , 0.841471 , 0.9092974], dtype=float32)
>>> np.cos(np.array([0, 1, 2], dtype=np.int8))
array([ 1.       ,  0.5403023, -0.4161468], dtype=float32)

@seberg
Copy link
Member Author

seberg commented Jan 10, 2023

Ah, good point, although I really don't like how we guess at float16 for integer inputs...

seberg added a commit to seberg/numpy that referenced this issue Jan 10, 2023
These were incorrect afer being vectorized.  The commit additional
tests these (not arctan2 admittedly) and adds a check to generate_umath
to make it a bit less likely that future additions add this type of thing.

Note that the check allows duplicated loops so long they are correctly
ordered the *first* time.  This makes results correct, but duplicated
loops are not nice anyways and it would be nice to remove them.

We could drop them manually in hindsight even?  In any case, that should
not be backported, so it is not includedhere.

Closes numpygh-22984
charris pushed a commit to charris/numpy that referenced this issue Jan 10, 2023
These were incorrect afer being vectorized.  The commit additional
tests these (not arctan2 admittedly) and adds a check to generate_umath
to make it a bit less likely that future additions add this type of thing.

Note that the check allows duplicated loops so long they are correctly
ordered the *first* time.  This makes results correct, but duplicated
loops are not nice anyways and it would be nice to remove them.

We could drop them manually in hindsight even?  In any case, that should
not be backported, so it is not includedhere.

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

Successfully merging a pull request may close this issue.

2 participants