ENH: Adding parameters to unwrap #14877
Conversation
Adding optional min_val and max_val to unwrap without changing normal behavior (discont is overwritten unless it is explicitly specified)
| @@ -1501,14 +1506,16 @@ def unwrap(p, discont=pi, axis=-1): | |||
| array([ 0. , 0.78539816, 1.57079633, -0.78539816, 0. ]) # may vary | |||
There was a problem hiding this comment.
An example here with integers would be good
There was a problem hiding this comment.
how about something like this
>>> unwrap([0, 1, 2, -1, 0], min_val=-1, max_val=3)
array([0., 1., 2., 3., 4.])
There was a problem hiding this comment.
You shouldn't need may vary, because it doesn't use floating point printing
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
improving documentation in response to input from @eric-wieser
|
@eric-wieser anything else I should do for this PR? |
| ddmod = mod(dd + pi, 2*pi) - pi | ||
| _nx.copyto(ddmod, pi, where=(ddmod == -pi) & (dd > 0)) | ||
| ddmod = mod(dd - min_val, max_val - min_val) + min_val | ||
| _nx.copyto(ddmod, max_val, where=(ddmod == min_val) & (dd > 0)) |
There was a problem hiding this comment.
Any idea what this ddmod == min_val condition is for? Can you produce a case that changes behavior based on whether this condition is here?
(I realize it was here before, but wonder if it was never hit due to floating point innacuracies)
There was a problem hiding this comment.
Not sure, I guess it might come up more with radian sequences and things actually equalling pi?
There was a problem hiding this comment.
Probably easier to investigate only the integer case, floating point rounding is just a distraction.
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
fixing underline
changing the input and adding explicit sequences
updating examples
| @@ -1499,16 +1507,19 @@ def unwrap(p, discont=pi, axis=-1): | |||
| array([ 0. , 0.78539816, 1.57079633, 5.49778714, 6.28318531]) # may vary | |||
| >>> np.unwrap(phase) | |||
| array([ 0. , 0.78539816, 1.57079633, -0.78539816, 0. ]) # may vary | |||
|
|
|||
| >>> unwrap([0, 1, 2, -1, 0], min_val=-1, max_val=3) | |||
| array([0., 1., 2., 3., 4.]) | |||
There was a problem hiding this comment.
This output should be integral, not floating-point
There was a problem hiding this comment.
Something is wrong with this function as implemented:
In [16]: unwrap([0, 1, 2, 3, 0, 1, 2, 3], min_val=0, max_val=4)
Out[16]: array([0., 1., 2., 3., 4., 5., 6., 7.]) # ok
In [17]: unwrap([1, 2, 3, 4, 1, 2, 3, 4], min_val=1, max_val=5)
Out[17]: array([1., 2., 3., 4., 5., 6., 7., 8.]) # ok
In [18]: unwrap([2, 3, 4, 5, 2, 3, 4, 5], min_val=2, max_val=6)
Out[18]: array([ 2., 3., 4., 5., 10., 11., 12., 13.]) # wrong?|
Closing. |
Adding optional min_val and max_val to unwrap without changing normal behavior (discont is overwritten unless it is explicitly specified).
I unfortunately not yet followed any of the guidelines and did the PR more to provide a simple reference implementation, I'll try to clean it up