RTD failure due to plots involving convolution Kernels · Issue #13209 · astropy/astropy · GitHub

Description

There are RTD failures due to a missing ndim in latest PRs, see:

I believe this may have to do with the latest version of matplotlib (https://github.com/matplotlib/matplotlib/releases/tag/v3.5.2) interacting poorly with how plots for Box1DKernel, Gaussian1DKernel, RickerWavelet1DKernel, and Trapezoid1DKernel are coded.

Note that a simple attempt at fixing this was tried in #13205; however, I think the fix will be more involved than this.

This is related to matplotlib/matplotlib#22973

@keflavich do you have any suggestions on how to fix this?

I am surprised passing in the whole kernel object even worked in the first place, as matplotlib has no way to know our kernel API. Was it a happy accident that it used to work? Should we really be passing in .array? Is this a regression that Matplotlib needs to fix upstream?

astropy/astropy/convolution/kernels.py

Lines 75 to 78 in 5ddd6a3

    
                   import matplotlib.pyplot as plt 
        
                   from astropy.convolution import Gaussian1DKernel 
        
                   gauss_1D_kernel = Gaussian1DKernel(10) 
        
                   plt.plot(gauss_1D_kernel, drawstyle='steps')

Does this has anything to do with the plot directive working before?

astropy/astropy/convolution/core.py

Lines 169 to 182 in 5ddd6a3

    
               def __array__(self): 
        
                   """ 
        
                   Array representation of the kernel. 
        
                   """ 
        
                   return self._array 
        
               def __array_wrap__(self, array, context=None): 
        
                   """ 
        
                   Wrapper for multiplication with numpy arrays. 
        
                   """ 
        
                   if type(context[0]) == np.ufunc: 
        
                       return NotImplemented 
        
                   else: 
        
                       return array

p.s. We cannot release with a broken doc build, so we either need to figure this out fast or pin maxversion of matplotlib for doc build for now.

p.p.s. Maybe @mhvk has insights into the __array* magicks?

The __array__ would certainly explain why it worked before - that is the standard way to define how one wants to be seen in an np.array(kernel) call. Given that wish for the kernel to be seen as array-like, adding .ndim, as tried in #13205, seems just be the right thing to do. So, if it works, I would go with that!

Unfortunately, as noted in #13205 (comment). Adding .ndim was not sufficient. It seems like __array__ is no longer working for matplotlib in this case. Any suggestions?

Can you point me to which doc is failing? I'd think we could just imshow(kernel.array) in the docs and call it done, unless it's really important that the __array__ magic works.

Can you point me to which doc is failing? I'd think we could just imshow(kernel.array) in the docs and call it done, unless it's really important that the __array__ magic works.

astropy/astropy/convolution/kernels.py

Lines 75 to 78 in 5ddd6a3

    
                   import matplotlib.pyplot as plt 
        
                   from astropy.convolution import Gaussian1DKernel 
        
                   gauss_1D_kernel = Gaussian1DKernel(10) 
        
                   plt.plot(gauss_1D_kernel, drawstyle='steps')

and the similar portions of each of the other kernels.

The __array__ magic is only a concern if it points at some sort of broken API on our end.

The plot thickens, I tried creating a minimal reproducer for this issue:

import numpy as np
import matplotlib.pyplot as plt

class Kernel:
    def __init__(self, array):
        self._array = np.asanyarray(array)
        
    def __array__(self):
        return self._array

x = np.array([1, 2, 3, 4, 5])
kernel = Kernel(x)
plt.plot(kernel)

which works. This was in the theory that matplotlib was not coercing the input anymore. But indeed since this example works (only with __array__ defined) we can conclude this is not the case.

I think the best path forward is to just explicitly plot the arrays rather than the objects themselves in the doc examples.

I think this is due to matplotlib/matplotlib#22141 (before that _check_1d was converting the input to a Numpy array with np.atleast_1d(x)). Probably worth a bug report.

Agreed it is worth a bug report. They now check for to_numpy() - if a class has __array__ that arguably should be used as well!

Reopening so this can be tracked for an upstream fix.

@WilliamJamieson - the difference from your minimal example and the real problem is, I think, that Kernel has a .shape defined and matplotlib sees this and treats the object as "sufficiently array-like"; without the shape, the object gets coerced to ndarray, and that works because of the __array__ method.

@WilliamJamieson - the difference from your minimal example and the real problem is, I think, that Kernel has a .shape defined and matplotlib sees this and treats the object as "sufficiently array-like"; without the shape, the object gets coerced to ndarray, and that works because of the __array__ method.

@mhvk you are exactly correct. See matplotlib/matplotlib#22973 (comment) where I just posted a better minimal reproducer. My above comment was me doing a first quick attempt.

OK if you want to track but given our pin in setup.cfg, this no longer blocks astropy v5.1. Thanks!

This has now been fixed by matplotlib/matplotlib#22975 so the next release of matplotlib should contain the needed fix.

RTD failure due to plots involving convolution Kernels · Issue #13209 · astropy/astropy · GitHub

Repository navigation

RTD failure due to plots involving convolution Kernels #13209

Description

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	import matplotlib.pyplot as plt
	from astropy.convolution import Gaussian1DKernel
	gauss_1D_kernel = Gaussian1DKernel(10)
	plt.plot(gauss_1D_kernel, drawstyle='steps')

	def __array__(self):
	"""
	Array representation of the kernel.
	"""
	return self._array

	def __array_wrap__(self, array, context=None):
	"""
	Wrapper for multiplication with numpy arrays.
	"""
	if type(context[0]) == np.ufunc:
	return NotImplemented
	else:
	return array

RTD failure due to plots involving convolution Kernels · Issue #13209 · astropy/astropy · GitHub

Repository navigation

RTD failure due to plots involving convolution Kernels #13209

Description

Description

Activity

WilliamJamieson commented on May 3, 2022

pllim commented on May 3, 2022

pllim commented on May 3, 2022

mhvk commented on May 3, 2022

WilliamJamieson commented on May 3, 2022

keflavich commented on May 3, 2022

WilliamJamieson commented on May 3, 2022

WilliamJamieson commented on May 4, 2022

saimn commented on May 4, 2022

mhvk commented on May 4, 2022

WilliamJamieson commented on May 4, 2022

mhvk commented on May 4, 2022

WilliamJamieson commented on May 4, 2022

pllim commented on May 4, 2022

WilliamJamieson commented on May 18, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions