You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This would allow unwrap to work on subclasses of ndarrays. deg2rad, rad2deg (etc.) are all ufuncs, so it's not readily apparent why unwrap is not a ufunc.
deg2rad and friends only need to look at one value at a time, so they fit
the classic ufunc model. unwrap is different: it needs to look at pairs of
values, so it would have to be a k->k gufunc. Which perhaps is a good idea
if it can be done in a backwards compatible way, but that's why it wasn't
done that way originally. (Probably unwrap predates the existence of
gufuncs.)
My potentially poorly informed understanding was that it needs to be a ufunc for it to work in subclasses. Ignoring that point, from a user standpoint unwrap is used commonly with angle, deg2rad, and rad2deg, so it's rather odd that unwrap would not behave in a similar manner. Granted, this is entirely from an outsider's point of view.
When I first started using MaskedArray's I hit this issue. Everything was working for taking the angle of a complex dataset up until I had to unwrap it.
It does seem this can be made to work on subclasses by replacing asarray with asanyarray, and replacing the copyto stanzas with settings corresponding slices (as copyto is not subclass-safe; that might in fact be a different function that could do with using a "selection ufunc"!)
You can't reliably make anything work with subclasses, because people write
subclasses with arbitrary semantics... Going through __array_ufunc__ is
much more robust when we can do it.
I couldn't agree more that __array_ufunc__ is better; just not sure that is a particularly easy way here (accumulate might work, but what is the actual ufunc, then? the gufunc is more logical, but needs my axis PR to be able to be a proper replacement).
I think a gufunc is a good option, though. The only issue is that it needs an axis argument -- though we could wing that by using the current wrapper for that.
It would be relatively straightforward to change unwrap to not downcast to ndarray, and use __setitem__ instead of copy to, which would fix most subclass use.
You're still not going to get meaningful results for masked arrays with that though - it's not even clear to me what the correct meaning is.
Masked arrays were just an example of how unwrap would not preserve the subclassing. I'm not expecting unwrap to work for masked arrays without modification. More, unwrap, just happens to be a very common operation with working with phasors over frequency, and all the other functions preserve the subclass.
Is this fix going to be incorporated? I hope it will.
That is now merged, should we make unwrap a gufunc? It would seem to fit the template of a ufunc more than a generic array operation since it has a well defined inner loop
Activity
eric-wieser commentedon Nov 4, 2017
Is there any need for it to be a ufunc? Or do you just want it to work on subclasses?
njsmith commentedon Nov 4, 2017
mcdevitts commentedon Nov 5, 2017
My potentially poorly informed understanding was that it needs to be a ufunc for it to work in subclasses. Ignoring that point, from a user standpoint unwrap is used commonly with angle, deg2rad, and rad2deg, so it's rather odd that unwrap would not behave in a similar manner. Granted, this is entirely from an outsider's point of view.
When I first started using MaskedArray's I hit this issue. Everything was working for taking the angle of a complex dataset up until I had to unwrap it.
eric-wieser commentedon Nov 5, 2017
@njsmith:
unwrapcan be modeled as aufunc.accumulatethough, with an inner loop something likeI don't understand the meaning of the
discontargument thoughmhvk commentedon Nov 5, 2017
It does seem this can be made to work on subclasses by replacing
asarraywithasanyarray, and replacing thecopytostanzas with settings corresponding slices (ascopytois not subclass-safe; that might in fact be a different function that could do with using a "selectionufunc"!)njsmith commentedon Nov 5, 2017
mhvk commentedon Nov 5, 2017
I couldn't agree more that
__array_ufunc__is better; just not sure that is a particularly easy way here (accumulatemight work, but what is the actualufunc, then? thegufuncis more logical, but needs myaxisPR to be able to be a proper replacement).charris commentedon Nov 5, 2017
The current ufunc setup is pretty constraining with all the various methods. We should take another look at the design at some point.
mhvk commentedon Nov 5, 2017
I think a
gufuncis a good option, though. The only issue is that it needs anaxisargument -- though we could wing that by using the current wrapper for that.eric-wieser commentedon Nov 5, 2017
It would be relatively straightforward to change
unwrapto not downcast to ndarray, and use__setitem__instead of copy to, which would fix most subclass use.You're still not going to get meaningful results for masked arrays with that though - it's not even clear to me what the correct meaning is.
mcdevitts commentedon Feb 19, 2018
Masked arrays were just an example of how unwrap would not preserve the subclassing. I'm not expecting unwrap to work for masked arrays without modification. More, unwrap, just happens to be a very common operation with working with phasors over frequency, and all the other functions preserve the subclass.
Is this fix going to be incorporated? I hope it will.
mattip commentedon Jun 13, 2018
That is now merged, should we make unwrap a gufunc? It would seem to fit the template of a ufunc more than a generic array operation since it has a well defined inner loop
mhvk commentedon Jun 13, 2018
That now is definitely possible!