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

Change in ufunc loop selection breaks some SciPy ufuncs. #20544

Closed
WarrenWeckesser opened this issue Dec 8, 2021 · 10 comments
Closed

Change in ufunc loop selection breaks some SciPy ufuncs. #20544

WarrenWeckesser opened this issue Dec 8, 2021 · 10 comments

Comments

@WarrenWeckesser
Copy link
Member

WarrenWeckesser commented Dec 8, 2021

Describe the issue:

Some scipy tests that run with the main git branch of numpy are failing because of an apparent change in the loop choice of some ufuncs.

An example is the ufunc scipy.special.bdtr. Here's a session where I'll demonstrate the issue.

In [1]: import numpy as np

In [2]: np.__version__
Out[2]: '1.23.0.dev0+129.gce4555cbf'

In [3]: import scipy

In [4]: scipy.__version__
Out[4]: '1.9.0.dev0+1038.bd4b732'

In [5]: from scipy.special import bdtr

In [6]: bdtr.types
Out[6]: ['fff->f', 'dld->d', 'ddd->d']

We want users to pass integers in the second argument; the signature dld->d represents the desired API.

The function also has a loop with signature ddd->d, but if this loop is used, a DeprecationWarning is generated.

In [7]: bdtr(1.0, 1, 0.5)  # No warning (as expected)
Out[7]: 1.0

In [8]: bdtr(1.0, 1.0, 0.5)  # Deprecation warning (as expected)
<ipython-input-8-c767f99dd632>:1: DeprecationWarning: non-integer arg n is deprecated, removed in SciPy 1.7.x
  bdtr(1.0, 1.0, 0.5)
Out[8]: 1.0

The new problem occurs with a call such as bdtr(1, 1, 0.5). Previously, this would use the dld->d loop. With the main git branch, it uses the ddd-d loop:

In [9]: bdtr(1, 1, 0.5)  # This is the change; the deprecation warning is new.
<ipython-input-9-4c509b7ce3aa>:1: DeprecationWarning: non-integer arg n is deprecated, removed in SciPy 1.7.x
  bdtr(1, 1, 0.5)
Out[9]: 1.0

I don't know if this is a numpy bug, or if the scipy code is relying on behavior that cannot be guaranteed by numpy.

Reproduce the code example:

See above.

Error message:

None.

NumPy/Python version information:

See above.

@seberg
Copy link
Member

seberg commented Dec 8, 2021

Arrrrggg :((, another pit to get stuck in. Yeah, if any promotion is necessary it would find the homogeneous loop first, which somewhat assumes that there is no such thing as a bad loop, just non-ideal ones. But apparently "bad" loops exists and we can't know which one :(.

But that is pretty darn annoying, does that mean we can't transition to new promotion by warning if the fallback is necessary?

@seberg seberg added the 00 - Bug label Dec 8, 2021
@seberg seberg added this to the 1.22.0 release milestone Dec 8, 2021
@seberg
Copy link
Member

seberg commented Dec 8, 2021

Well... I guess I can probably just get away with just removing most (all?) of the promotion, because I had to put in the more explicit reduce-workaround later and now it is probably unnecessary for the time being. An oversight on my part, sometimes is hard to see things if you dove too deep...

I would like to take a moment to think about possible transitions, but I guess that may just be to force you to transition to a new API at some point (which is not available yet as it is fully experimental).

@charris
Copy link
Member

charris commented Dec 8, 2021

Guess 1.22.0rc2 won't go out today :) @WarrenWeckesser Did this error just appear today, or was it there earlier. I ask because I triggered a new nightly yesterday.

@WarrenWeckesser
Copy link
Member Author

The problem occurred in scipy/scipy#15176. The CI check where the errors showed up is Linux Tests / Nightly CPython (3.10) (pull_request). That check uses a git checkout of the main branch of numpy, not a nightly numpy.

@seberg
Copy link
Member

seberg commented Dec 8, 2021

@charris yeah, it is definitely due to the new changes, because of the addition of a new "default promoter" that was not there before. I guess I just have to revert (that part of it). On the up-side, it deletes a lot of stuff for now ;).

@seberg
Copy link
Member

seberg commented Dec 8, 2021

Could be interesting to see if anyone else notices ;).

@seberg
Copy link
Member

seberg commented Dec 8, 2021

Well, pandas noticed, because np.arange(10, dtype="m8[ns]").sum(dtype="f8") dares to actually return datetimes and ignore the dtype request apparently. And somehow apparently I changed it (although I am still a bit confused why that happend, probably some dubious promotion of m8 and float?).

@seberg
Copy link
Member

seberg commented Dec 9, 2021

Should be fixed for the time being. Thanks. Hopefully, it is now good enough for the 1.22 release.

@seberg seberg closed this as completed Dec 9, 2021
@WarrenWeckesser
Copy link
Member Author

WarrenWeckesser commented Dec 9, 2021

Thanks @seberg, the SciPy CI check that runs with numpy's main development branch is now passing again.

@seberg
Copy link
Member

seberg commented Dec 9, 2021

Thanks! I really hope (and think) that things should be de-facto back to before. (With the only exception that occasionally you may get a slightly more precise loop for reductions when it probably makes sense.)

Unfortunately, it means we still have most of the old logic around, just repackaged a bit...

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

No branches or pull requests

3 participants