Repository navigation
[Bug]: v3.5.2 causing plot to crash when plotting object with __array__ method #22973
Description
Bug summary
In astropy/astropy#13209 a potential bug with v3.5.2 has been found wherein a previously working (e.g. in v3.5.1) plot of one of kernel objects now fails. This failure seems to stem from plot no-longer coercing the the kernel object correctly. This seems to be related to the changes in #22141. In any case we have an object with a valid __array__ method which was previously plotting correctly, but now produces a crash.
Code for reproduction
import matplotlib.pyplot as plt
from astropy.convolution import Gaussian1DKernel
gauss_1D_kernel = Gaussian1DKernel(10)
plt.plot(gauss_1D_kernel, drawstyle='steps')Actual outcome
Traceback (most recent call last):
File "/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/12943/lib/python3.9/site-packages/matplotlib/sphinxext/plot_directive.py", line 517, in _run_code
exec(code, ns)
File "<string>", line 4, in <module>
File "/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/12943/lib/python3.9/site-packages/matplotlib/pyplot.py", line 2769, in plot
return gca().plot(
File "/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/12943/lib/python3.9/site-packages/matplotlib/axes/_axes.py", line 1632, in plot
lines = [*self._get_lines(*args, data=data, **kwargs)]
File "/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/12943/lib/python3.9/site-packages/matplotlib/axes/_base.py", line 312, in __call__
yield from self._plot_args(this, kwargs)
File "/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/12943/lib/python3.9/site-packages/matplotlib/axes/_base.py", line 500, in _plot_args
if x.ndim > 2 or y.ndim > 2:Expected outcome
See: https://docs.astropy.org/en/latest/_images/astropy-convolution-Gaussian1DKernel-1.png
Additional information
No response
Operating system
No response
Matplotlib Version
3.5.2
Matplotlib Backend
No response
Python version
No response
Jupyter version
No response
Installation
pip
Activity
jklymak commented on May 4, 2022
OK, sorry about that - its pretty hard to track all the ways we can coerce to numpy, and we are working hard to consolidate them. Can astropy make a mocked up class for us that we can use for testing that should work? Currently looks like we are supporting to_numpy and then calling np.atleast_1d for some cases, but not doing it always.
jklymak commented on May 4, 2022
Its pretty easy to add a check for __array__ to the fallbacks. I think it just becomes an issue of what takes precedence if an object has multiple array accessors. My knee jerk would be:
to_numpy > values > __array__, but I'm not sure if there is a proper convention here.
tacaswell commented on May 4, 2022
I think __array__ should be first.
pllim commented on May 4, 2022
I am going to ping @keflavich and @mhvk here in case they have insights on astropy interface.
jklymak commented on May 4, 2022
The private before the public?
tacaswell commented on May 4, 2022
I would say actual protocol rather than ad-hoc protocol.
mhvk commented on May 4, 2022
@jklymak - __array__ is the standard protocol for allowing anything to be coerced to a numpy array - it is was np.array looks for too (which is why np.atleast_1d worked - it uses np.asanyarray which is short for np.array(<input>, subok=True, copy=False). It may be worth checking whether the pandas object doesn't support it already - I wouldn't be surprised...
jklymak commented on May 4, 2022
Thanks, that makes sense, though pandas definitely was failing, so we had to add the unpacking here (I don't know if you saw the diff for #22141 for what we were doing before that PR?)
We are completely untested on custom __array__ objects, so it would be very helpful if someone knowledgeable could mock one up for us just so we don't break this ability in the future.
We are always delicate about what point we coerce to numpy because sometimes objects we get passed have units, and we don't want to strip those out too early. Hence the desire to consolidate all this, and not rely on np.(as)(any)array in a blanket sense. But any advice on how to do this properly is gratefully received.
jklymak commented on May 4, 2022
For instance, the fix in #22975 fails the unit interface tests....
mhvk commented on May 4, 2022
@jklymak - very much appreciated that you try to remove too much explicit casting to numpy arrays! It seems reasonable to leave it to objects that are passed in to try to be good enough duck types.
WilliamJamieson commented on May 4, 2022
Here is a minimal reproducer class based on astropy.convolution.core.Kernel:
jklymak commented on May 4, 2022
@WilliamJamieson Thanks a lot. We can base a test around this, and make sure we don't break you (and others presumably) in the future. And again, sorry for the hassle.
oscargus commented on May 5, 2022
Just linking with #22645
to_numpy seems not needed anymore