-
-
Notifications
You must be signed in to change notification settings - Fork 11.9k
ENH: Allow no conversion to scalar guarantee in ufunc and ufunc.outer #14489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
86dc281 to
445c31d
Compare
|
Would be great if we could add this for reductions too - we don't want to end up in a limbo where |
|
Well, I am still not sure if I don't think that The change itself is going to be easy though, I think. |
I'm not suggesting we change the default any time soon, just that we give users the option to leave things wrapped for reductions too. |
|
Yeah, I was considering for a moment if |
|
Hmmmpf, there is an issue. Implementors of The other option would be to guarantee an array (EDIT: read |
|
@mhvk since I migrated a bit from the issue. just in case you are interested. Reduction is not an issue of course. The main issue is that |
d490a16 to
2d98a62
Compare
I think if this happens, we should just throw an exception - since |
|
I'd like to add this to triage review. |
|
We could also use |
|
Now that's an interesting idea. I almost wonder if we want to allow passing any array subclass in there. |
|
@eric-wieser well, we could initially only support ndarray. The thing I think that is missing is to figure out where we want to use this right now. I think it may be that almost(?) all places where we would be using this internally, we already make sure we are wokring with arrays. But yes, I like the idea as such to use the type... The issue is that we have no control over what |
|
Passing in |
The current commit uses ... which is short to write, but other than that does not have much reason going for. So we should probably change that.
|
In this respect, perhaps the original |
I think the point you're missing is that |
|
Hmm, indeed. So I guess your solution makes more sense. I do wonder whether instead of creating a new keyword argument for what is a niche case instead always pass on a context, also for ufunc reductions, and include |
|
@eric-wieser triage marking is nice, but I need a bit more head-start to bring it up (and it would be best if you are around too). I still have to get to wrapping my head around the options, I guess the only way to support array-wrap is to add the argument. Which was why I think the PR basically used |
|
@seberg is this meant to be part of the ufunc reworking for 1.20 or do we let it slide to a future release? |
|
Let it slide, it might create ufunc rework merge conflicts, but is an unrelated feature. Should look at it and see if I can figure out how subclasses should be dealt with though :/. |
|
@seberg It looks like your recent work leads to a conflict. |
|
@seberg Needs rebase. |
|
@seberg Still interested in this? Needs rebase. |
|
It sounds like there is some serious thought about a 2.0. Maybe in that case we can just start to return array scalars instead of proper scalars? I.e., start to omit the cast to scalar? Apart from being mutable, the difference seems rather small. |
|
Does numpy 2.0 include deprecating scalars in favour of array scalars? If so, this becomes somewhat moot, but I didn't see it on the roadmap |
|
I don't want to aim for it. For me there are still quite a lot of unknowns and probably some exploration needed about how feasible it is. |
|
In that case, it is perhaps worth revising this PR? |
|
Yes, likely. Still haven't had the spark of the original idea how to deal with subclasses. We could also abuse An alternative thing that would be a big change, but not as big as removing scalars one, would be this: https://github.com/seberg/numpy/pull/new/try-0d-preservation that I tried to explore a bit today. The proposal would be to:
(The tail of that is not super trivial, quite a few test failures, scipy also relies on scalars in a few places for example, but it looks harmless). But run that branch with: to try. A fun little caveat: |
|
This would still be nice to have, though I now wonder if numpy 2.0 isn't a good opportunity to just change it -- really, if one gives arrays as input, one should get arrays as output, even if the arrays happen to have shape |
|
We discussed this in a triage meeting and decided to close this but leave the issue open, maybe we will revisit for 3.0 |
|
While I don't mind closing this, it has nothing to do with a 3.0 release. That would b changing the behavior, which I think we can and should also pull off. |
As of now a WIP, because it would be nice to see what `_methods.py`. The API change is to allow `out=...`, for multi-argument outputs it is currently `out=(..., ...)`, although I guess we should allow short-handing that (nobody needs to pick it per argument). There is some discussion in numpygh-14489 about this. I still think `...` is the best, because `out=np.ndarray` doesn't generalize for `__array_ufunc__`, and yes, I like `...` because it also indicates an array return in indexing. (`arr[i, ...]` always returns an array.) --- The ufunc part works (not sure if 100% complete), but the `_methods.py` changes do not (and change behavior subtlely, if possibly correctly). The actual change is trivial (after I had refactored this a while ago). I think we should do this, but the fact that the `_methods.py` changes are not trivial makes me think there may be some idea missing. It may be that the solution is for these functions to use conversion helper once at the start and then convert to scalars based on that at the end though (rather than dragging around subclasses).
As of now a WIP, because it would be nice to see what `_methods.py`. The API change is to allow `out=...`, for multi-argument outputs it is currently `out=(..., ...)`, although I guess we should allow short-handing that (nobody needs to pick it per argument). There is some discussion in numpygh-14489 about this. I still think `...` is the best, because `out=np.ndarray` doesn't generalize for `__array_ufunc__`, and yes, I like `...` because it also indicates an array return in indexing. (`arr[i, ...]` always returns an array.) --- The ufunc part works (not sure if 100% complete), but the `_methods.py` changes do not (and change behavior subtlely, if possibly correctly). The actual change is trivial (after I had refactored this a while ago). I think we should do this, but the fact that the `_methods.py` changes are not trivial makes me think there may be some idea missing. It may be that the solution is for these functions to use conversion helper once at the start and then convert to scalars based on that at the end though (rather than dragging around subclasses).
* API,ENH: Allow forcing an array result in ufuncs This allows using `out=...` for ufuncs (including reductions) to enforce an array result. With it, the result will never be converted to a scalar, but subclasses are preserved normally. (A subclass might still misbehave and convert to a scalar anyway, but it would be a bug there.) There is some discussion in gh-14489 about this. I still think `...` is the best, because `out=np.ndarray` doesn't generalize for `__array_ufunc__`, and yes, I like `...` because it also indicates an array return in indexing. (`arr[i, ...]` always returns an array.) It uses this in a few places, although there are many more, e.g. `_methods.py` should probably use it, but it is very subtle there. (Also because some paths are probably subtly wrong for object dtype.) --------- Signed-off-by: Sebastian Berg <[email protected]> Co-authored-by: Marten H. van Kerkwijk <[email protected]> Co-authored-by: Nathan Goldbaum <[email protected]>
* API,ENH: Allow forcing an array result in ufuncs This allows using `out=...` for ufuncs (including reductions) to enforce an array result. With it, the result will never be converted to a scalar, but subclasses are preserved normally. (A subclass might still misbehave and convert to a scalar anyway, but it would be a bug there.) There is some discussion in numpygh-14489 about this. I still think `...` is the best, because `out=np.ndarray` doesn't generalize for `__array_ufunc__`, and yes, I like `...` because it also indicates an array return in indexing. (`arr[i, ...]` always returns an array.) It uses this in a few places, although there are many more, e.g. `_methods.py` should probably use it, but it is very subtle there. (Also because some paths are probably subtly wrong for object dtype.) --------- Signed-off-by: Sebastian Berg <[email protected]> Co-authored-by: Marten H. van Kerkwijk <[email protected]> Co-authored-by: Nathan Goldbaum <[email protected]>
The current commit uses
...(Ellipsis) instead of aleave_wrappedsingleton, which is short to write, but other thanthat does not have much reason going for. So we should probably
change that.
xref gh-13105