Repository navigation
CCDData raises KeyError when passed into numpy functions on numpy 1.24 #15397
Description
Description
After installing astropy earlier today, I ran into an issue that I hadn't seen before. When passing a CCDData instance into np.min(), I receive a vague KeyError.
Expected behavior
In numpy 1.23/astropy 5.1, np.min(CCDData) would return the minimum value of the CCDData array.
Is this expected behavior?
Even if np.min(CCDData) should not be allowed due to its handling of units, the exception produced does not indicate that.
How to Reproduce
mamba install astropy numpy
import numpy as np
from astropy import nddata
a=nddata.CCDData([1,2,3],unit="adu")
np.min(a)expected / old result:
1 (notably, units were not preserved)
new result:
---------------------------------------------------------------------------
KeyError Traceback (most recent call last)
File ~/miniforge3/envs/mamba_test/lib/python3.11/site-packages/astropy/nddata/mixins/ndarithmetic.py:243, in NDArithmeticMixin._arithmetic(self, operation, operand, propagate_uncertainties, handle_mask, handle_meta, uncertainty_correlation, compare_wcs, operation_ignores_mask, axis, **kwds)
242 try:
--> 243 kwds2[splitted[0]][splitted[1]] = kwds[i]
244 except KeyError:
KeyError: 'out'
During handling of the above exception, another exception occurred:
KeyError Traceback (most recent call last)
Cell In[10], line 1
----> 1 np.min(a)
File <__array_function__ internals>:200, in amin(*args, **kwargs)
File ~/miniforge3/envs/mamba_test/lib/python3.11/site-packages/numpy/core/fromnumeric.py:2946, in amin(a, axis, out, keepdims, initial, where)
2829 @array_function_dispatch(_amin_dispatcher)
2830 def amin(a, axis=None, out=None, keepdims=np._NoValue, initial=np._NoValue,
2831 where=np._NoValue):
2832 """
2833 Return the minimum of an array or minimum along an axis.
2834
(...)
2944 6
2945 """
-> 2946 return _wrapreduction(a, np.minimum, 'min', axis, None, out,
2947 keepdims=keepdims, initial=initial, where=where)
File ~/miniforge3/envs/mamba_test/lib/python3.11/site-packages/numpy/core/fromnumeric.py:84, in _wrapreduction(obj, ufunc, method, axis, dtype, out, **kwargs)
82 return reduction(axis=axis, dtype=dtype, out=out, **passkwargs)
83 else:
---> 84 return reduction(axis=axis, out=out, **passkwargs)
86 return ufunc.reduce(obj, axis, dtype, out, **passkwargs)
File ~/miniforge3/envs/mamba_test/lib/python3.11/site-packages/astropy/nddata/mixins/ndarithmetic.py:644, in NDArithmeticMixin.min(self, **kwargs)
640 @sharedmethod
641 def min(self, **kwargs):
642 # use the provided propagate_uncertainties if available, otherwise default is False:
643 propagate_uncertainties = kwargs.pop("propagate_uncertainties", None)
--> 644 return self._prepare_then_do_arithmetic(
645 np.min, propagate_uncertainties=propagate_uncertainties, **kwargs
646 )
File ~/miniforge3/envs/mamba_test/lib/python3.11/site-packages/astropy/nddata/mixins/ndarithmetic.py:746, in NDArithmeticMixin._prepare_then_do_arithmetic(self_or_cls, operation, operand, operand2, **kwargs)
737 result, init_kwds = cls._arithmetic(
738 operand,
739 operation,
740 operand2,
741 **kwargs,
742 )
743 else:
744 # otherwise call the _arithmetic method on self for a collapse operation:
745 # for collapse operations, use astropy.utils.masked rather than handle_mask
--> 746 result, init_kwds = self_or_cls._arithmetic(
747 operation,
748 operand2,
749 **kwargs,
750 )
752 # Return a new class based on the result
753 return cls(result, **init_kwds)
File ~/miniforge3/envs/mamba_test/lib/python3.11/site-packages/astropy/nddata/mixins/ndarithmetic.py:245, in NDArithmeticMixin._arithmetic(self, operation, operand, propagate_uncertainties, handle_mask, handle_meta, uncertainty_correlation, compare_wcs, operation_ignores_mask, axis, **kwds)
243 kwds2[splitted[0]][splitted[1]] = kwds[i]
244 except KeyError:
--> 245 raise KeyError(f"Unknown prefix {splitted[0]} for parameter {i}")
247 kwargs = {}
249 # First check that the WCS allows the arithmetic operation
KeyError: 'Unknown prefix out for parameter out'Versions
Linux-4.19.294-1-MANJARO-x86_64-with-glibc2.38
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:34:09) [GCC 12.3.0]
astropy 5.3.3
Numpy 1.24.4
pyerfa 2.0.0.3
Scipy 1.11.2
Matplotlib 3.8.0
Activity
github-actions commentedon Sep 27, 2023
Welcome to Astropy 👋 and thank you for your first issue!
A project member will respond to you as soon as possible; in the meantime, please double-check the guidelines for submitting issues and make sure you've provided the requested details.
GitHub issues in the Astropy repository are used to track bug reports and feature requests; If your issue poses a question about how to use Astropy, please instead raise your question in the Astropy Discourse user forum and close this issue.
If you feel that this issue has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].
pllim commentedon Sep 27, 2023
That is because
np.minhas "out" now and that signature is somehow passed intokwdshere and crashing:astropy/astropy/nddata/mixins/ndarithmetic.py
Lines 240 to 245 in e6ff15a
But then again, I am not sure if this is a supported usage. Our CI does not run this (hence we did not catch this incompatibility).
Why not use it like this?
pllim commentedon Sep 27, 2023
@mwcraig , is
np.min(CCDData)supported?NanoExplorer commentedon Sep 27, 2023
I think it makes sense to use
a.min()when possible. Right now it's another library that's usingnp.min(a). I can send them a pull request to switch.If it isn't supported, would it be possible to make that clear from the exception?
pllim commentedon Sep 27, 2023
I'll have to defer to @mwcraig or @bmorris3 on this one.
Though when they lost unit using
np.min(a)to begin with, it would have been a strong hint that the usage is wonky?bmorris3 commentedon Sep 27, 2023
The numpy functions should be supported by NDData (or at least they were?), because many of numpy functions like
np.min(a)check for a built-in method with the same name on the the input object , likea.min(). Here's where that happens in the numpy source. So numpy will use the built-in implementation on NDData when NDData are passed and that method is available.The traceback in this issue arises from an unexpected call signature, which is still surprising to me. @pllim – did I understand your comment (#15397 (comment)) correctly that the
numpy.mincall signature changed?pllim commentedon Sep 27, 2023
It did change but maybe not "out". I vaguely remember discussion about "where" but even that was a while ago.
https://numpy.org/devdocs/reference/generated/numpy.min.html
mhvk commentedon Sep 27, 2023
The difference indeed seems to be that
np.minpasses onout=None. I think it can be solved fairly easily by addingout=Noneto the_arithmeticmethod (and raising ifout is not None; this is done inTimetoo.).Note that in principle people can also pass
keepdims, so perhaps that needs some code too...pllim commentedon Sep 27, 2023
Still, this usage was never tested in the CI. Hmm...
pllim commentedon Sep 27, 2023
Also, even if you add
outandkeepdimsto the mixin, there is still a matter of losing unit.mwcraig commentedon Sep 28, 2023
@NanoExplorer -- thanks for the detailed report!
I don't recall ever intending that
np.minwoudn't work, though at the time the code was written using.min()was the main way we expected folks to do it.With the new xarray standard we are going to need to find a way to support functions like
np.minas well as we have methods in the past.It seems like the short-term fix should be to implement what @mhvk suggested -- I can aim to do that over the weekend, but if you want to give it a shot, @NanoExplorer, let me know.
Not sure how to handle the units....