Make unwrap a ufunc

[mcdevitts]

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.

[eric-wieser]

Is there any need for it to be a ufunc? Or do you just want it to work on subclasses?

[njsmith]

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.)

…

On Nov 4, 2017 2:26 PM, "Eric Wieser" ***@***.***> wrote: Is there any need for it to be a ufunc? Or do you just want it to work on subclasses? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9959 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOaND-puEFYfwbIgEKojDapqZ4wfv2ks5szNZsgaJpZM4QSGpm> .

[mcdevitts]

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]

@njsmith: unwrap can be modeled as a ufunc.accumulate though, with an inner loop something like

def unwrap_inner(a, b):
    return a + ieeeremainder(b - a, pi)

I don't understand the meaning of the discont argument though

[mhvk]

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"!)

[njsmith]

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.

…

On Nov 5, 2017 08:51, "Marten van Kerkwijk" ***@***.***> wrote: 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 are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9959 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOaNDb1ymG1DR-iZZJ03IO6lqFe-85ks5szeekgaJpZM4QSGpm> .

[mhvk]

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).

[charris]

just not sure that is a particularly easy way here

The current ufunc setup is pretty constraining with all the various methods. We should take another look at the design at some point.

[mhvk]

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.

[eric-wieser]

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.

[mcdevitts]

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]

The only issue is that it needs an axis argument

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]

That now is definitely possible!