Avoid creating results with numpy scalars (re: NEP 51) #7282
Conversation
There was a problem hiding this comment.
Overall I'm +1 on making this change - I think it's in line with what most users expect and it's worth the explicit conversions to ensure everything is numpy scalar-free.
On the specifics, there are some cases where the .tolist approach could be replaced with a pattern that makes the intent more explicit, but there are others where tolist seems like the right move (but could perhaps use a comment to explain why it's being called). See individual comments for details + further discussion!
Thanks for finding and sifting through these @eriknw , I had never considered taking the next step and actually getting rid of numpy scalars from return types!
There was a problem hiding this comment.
This looks good to me.
I have some comments below -- mostly silly stuff like comments and variable names, etc.
In general, wherever it is not clear why we're using tolist it is probably helpful to have a comment saying that tolist is converting to a python object.
Just a note here of something I learned:
If you have a numpy scalar, but don't know whether to convert to e.g. a python float or int, it looks like np.ndarray.item() is the way to convert 0-d and scalars to python objects. tolist says it uses this to actually do the conversion of the values to python numbers.
I'm approving this because these suggestions are very minor.
There was a problem hiding this comment.
Thanks for these comments -- I've replied below to a couple below. Looking good.
Also, just to make sure I have it clear in my head. We are not trying to convert all numpy arrays to python lists. We are only trying to convert numpy scalars to python numbers. No need to answer this unless we are trying to convert numpy arrays to lists. :)
|
Updated to use
I'll answer anyway... correct, we are only converting NumPy scalars to Python objects. NumPy array return types (ndim > 0) remain the same. |
* Avoid creating results with numpy scalars (re: NEP 51) * Revert changes to `convert_matrix.py` to see what happens * Unrevert reversion * Better * respond to feedback: be more clear and use `x.item()` * Use `a.item(i)` where appropriate
NEP 51 coming in NumPy 2..0 changes the repr of numpy scalar, and this helps reveal usage of numpy scalars where Python scalars ought to be used. This PR aims to allow the workaround in #6856 to be reverted.
This uses dispatching machinery to perform rigorous inspection of return values. Hence, the blind spots are functions that are not dispatchable. We probably shouldn't keep this code when we merge. Should we look into a more flexible callback machinery to allow this check (and other checks) to be optionally done? Inspecting return values has proven quite useful.