API: Ensure np.vectorize outputs can be subclasses. #19356
Conversation
As is, this is true for the ufunc case, but not for the gufunc case, even if the underlying function does produce a subclass. Given the care taken to ensure inputs are kept as subclasses, this is almost certainly an oversight, which is here corrected.
mhvk
force-pushed
the
functionbase-vectorize-refactor
branch
from
96b8df0
to
3c892cd
Compare
|
Hmm, I wonder if this is chasing the unreachable, and the correct thing would be to properly implement gh-14020... I.e. we don't yet call On the other hand: it seems fine to just do this as a step in the right direction. But if that seems better, I could also clock a bit of time on gh-14020, since I don't think it should be particularly difficult. |
|
Even I was wondering about chasing too far, but in this case the solution is both very small and seemed so obviously consistent with what the code that was there was trying to do anyway, that I thought I might as well push it up. p.s. An advantage over the approach to make the function a gufunc is that if the function already deals with subclasses, this implementation deals with it correctly on its own - for Anyway, long story short, let's not let the perfect be the enemy of the good (enough)! |
There was a problem hiding this comment.
The code changes look good. As far as I can tell:
- This will fail if
broadcast_shape = (0,)since you don't have anyresultsavailable. - It means you explicitly diverge further from normal gufuncs (in a way that gufuncs can probably never support).
I am happy with this if you think it is practical, though. We probably cannot make np.vectorize(..., signature=...) a proper ufuncs in any case, and even getting it much closer will be hard.
I.e. I think we may be able to grow signature and maybe proper dtype support to np.frompyfunc (if the user is explicit), but that won't really help with making np.vectorize more of a ufunc...
| arrays = tuple(np.empty(shape, dtype=dtype) | ||
| for shape, dtype in zip(shapes, dtypes)) | ||
| if dtypes is None: | ||
| dtypes = [None] * len(shapes) |
There was a problem hiding this comment.
This branch does not make sense if results is None, maybe just move it into that branch?
There was a problem hiding this comment.
I agree that in practice it is not currently possible to get here, since it either gets called with results or with explicit dtypes, but in the context of this helper function it seems clearer to just allow dtypes=None for both cases (and I need a list to be able to iterate over them).
If you think it better, though, since the function is now effectively different for the two paths insize vectorize, I could just inline it there.
There was a problem hiding this comment.
Doesn't matter, I just noticed it. What made me pause was just that in that path dtype=None effectively gives dtype=np.dtype(None) which is float64.
|
On the larger points: for I'm not sure, though, that this is diverging from |
|
I'm inclined to put this in. Comments? @mhvk What about the |
|
@charris - thanks, I think @seberg and I converged there, that in the context of that routine it was fine. EDIT: In practice, the default of |
|
Yeah, its fine. It was just a "take it or leave it" comment. If you are also inclined to put it in, lets give it a shot. Thanks Marten! |
As is, this is true for the ufunc case, but not for the gufunc case,
even if the underlying function does produce a subclass. Given the
care taken to ensure inputs are kept as subclasses, this is almost
certainly an oversight, which is here corrected.
p.s. Not quite sure whether to count this as a bugfix or API change, given that the element-wise part of
vectorizealready correctly dealt with subclasses.xref: astropy/astropy#11893