Skip to content

MNT: avoid future incompatibility with numpy 1.25 (casting single element arrays to scalar types) #25745

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

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Apr 21, 2023

PR Summary

fix the one issue in #25744 that I know how to reproduce. There might be more (I'll open this for review after I'm done discovering and fixing them).

I'm not sure how to document this kind of change for release notes, I'd appreciate guidance there.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@neutrinoceros
Copy link
Contributor Author

I ran the test suite locally with bleeding edge numpy installed + pytest -Werror and didn't find any other deprecation warnings. I think this now closes #25744

@neutrinoceros neutrinoceros marked this pull request as ready for review April 21, 2023 12:14
@anntzer
Copy link
Contributor

anntzer commented Apr 21, 2023

It would seem like the spirit of numpy's deprecation is more that we should also move towards not supporting passing single-element arrays here...

@neutrinoceros
Copy link
Contributor Author

Should I throw a deprecation warning in for that case ?

@jklymak
Copy link
Member

jklymak commented Apr 21, 2023

Why would someone pass in a single element array here anyhow? Can we not just let numpys warning suffice?

@neutrinoceros
Copy link
Contributor Author

for context, I'm not advocating that this is a reasonable use case. I've discovered this because it was triggered by the following line in yt :
https://github.com/yt-project/yt/blob/d73939ec7f13d9028cad8603f6d8302a5a61a8d8/yt/visualization/volume_rendering/transfer_functions.py#L690
... for which I also issued a patch: yt-project/yt#4422

If you guys are not interested in patching it in matplotlib, feel free to just close this alongside the issue I opened.

@jklymak
Copy link
Member

jklymak commented Apr 21, 2023

I think we are just trying to understand the issue to help decide the best way forward.

Why, in the linked code, would visible[0] return an array?

The ways forward are 1) work around this silently, 2) Deprecate on our end 3) just let the numpy Deprecation stand. I agree with @anntzer that we should do 2 or 3. I'd suggest we only do 2 if we think there is a reasonable use case where folks would have done this.

@tacaswell
Copy link
Member

$ ipython
iPython 3.12.0a7+ (heads/main:4898415df7, Apr 20 2023, 18:22:31) [GCC 12.2.1 20230201]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.13.0.dev -- An enhanced Interactive Python. Type '?' for help.

In [1]: import numpy as np

In [2]: np.array(-1)
Out[2]: array(-1)

In [3]: float(np.array(-1))
Out[3]: -1.0

In [4]: float(np.array([-1]))
<ipython-input-4-6635e9e78cfc>:1: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.)
  float(np.array([-1]))
Out[4]: -1.0

In [5]: np.__version__
Out[5]: '1.25.0.dev0+1205.g17bae7c3f'

In [6]:

I am 👎🏻 on taking this patch. I think that single element arrays (as opposed to 0-d "arrays") ever worked in this context is an accident of numpy's implementation.

I agree with @jklymak that in the yt example something else is funny. From the names my guess is that the blah.y attribute is a (N, 1) array (so it broadcasts nicely with a (1, M) array for the x?). I suspect a better fix in yt is to do the squeeze so that values is indeed a (N, ) array earlier in the code.

@neutrinoceros
Copy link
Contributor Author

Why, in the linked code, would visible[0] return an array?

good point. What's happening is that visible is created there https://github.com/yt-project/yt/blob/d73939ec7f13d9028cad8603f6d8302a5a61a8d8/yt/visualization/volume_rendering/transfer_functions.py#L639

and even though self.alpha.y is a 1D array, the result from np.argwhere is 2D (where the second dimension is 1). There's no question that I need to patch this in yt, but I wonder how likely it is that other libraries might be doing something similar and break unexpectedly (probably not very).

Again, no worries if you guys want to discard this as out of scope. :-)

@tacaswell
Copy link
Member

I'm going to decline this PR. As I said above, that this ever worked was an accident and we should trust Numpy's judgement that this is a behavior change we want.

The right place for this to be fixed is in the call site passing this to us (either user code or other down-stream libraries).

Thank you for bringing this up @neutrinoceros . I normally say "we hope to hear from you again", but when we hear from you it is likely because we broke yt and I hope we do not do that ;)

@tacaswell tacaswell closed this Apr 21, 2023
@neutrinoceros neutrinoceros deleted the handle_np_125_depr_single_elem_array_casting branch April 21, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants