Skip to content
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

np.clip with complex input is untested and has odd behavior #15630

Open
rgommers opened this issue Feb 23, 2020 · 9 comments · May be fixed by #15643
Open

np.clip with complex input is untested and has odd behavior #15630

rgommers opened this issue Feb 23, 2020 · 9 comments · May be fixed by #15643

Comments

@rgommers
Copy link
Member

@rgommers rgommers commented Feb 23, 2020

This came up in pytorch/pytorch#33568. This seems odd:

>>> np.clip([3 + 4.j], -1, 2)                                                                                       
array([2.+0.j])
>>> np.clip([3 + 4.j], -1+1.j, 2+12.j)  # imaginary component goes up                                                                   
array([2.+12.j])
>>> np.clip([1 + 4.j], -1+1.j, 2+12.j)  # imaginary component doesn't go up                                                                             
array([1.+4.j])

The only test for complex input is that it doesn't segfault.

Reasonable behavior could be one of:

  • Clip real and imaginary parts separately
  • Clip absolute value, not changing the phase
    There may be other options. As long as it's documented, I'm not sure it matters much which choice is made.

I don't think this is a very important issue, but it would be nice to at least have the desired behavior documented here.

birm added a commit to birm/numpy that referenced this issue Feb 25, 2020
birm added a commit to birm/numpy that referenced this issue Feb 25, 2020
@eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Feb 25, 2020

The current behavior is to makex = sorted(x) for x = [min, clip(arr, min, max), max], I think.

birm added a commit to birm/numpy that referenced this issue Feb 25, 2020
@birm birm linked a pull request that will close this issue Feb 25, 2020
@seberg
Copy link
Member

@seberg seberg commented Feb 25, 2020

I doubt it is valuable to break consistency with min, max, sorting here for a non-obvious behaviour choice. Not that we can't do it, but than we should probably also discuss min/max and sorting behaviour...
And if that is to say that an absmin, absmax, and abssort would make sense and basically use that for clipping.
I could be more easily convinced to just giving an error for non-real input.

EDIT: Arguably componentwise clipping actually allows for Erics behaviour, since it is more strict. But just a note, I am not sure I buy it as being useful. The other question is: is there any actual usecase for this at all? One that is not clearer with different code?

@charris
Copy link
Member

@charris charris commented Feb 25, 2020

Clip real and imaginary parts separately

That would be my first choice.

Clip absolute value, not changing the phase

Could be useful but is more complicated to implement than a simple clip.

@numpy numpy deleted a comment from charris Feb 26, 2020
@numpy numpy deleted a comment from charris Feb 26, 2020
@rgommers
Copy link
Member Author

@rgommers rgommers commented Feb 26, 2020

The other question is: is there any actual usecase for this at all? One that is not clearer with different code?

That's a good question. I'm not sure. All I know is it shouldn't be so obviously incorrect.

@mruberry
Copy link

@mruberry mruberry commented Apr 10, 2020

I'm worried about clipping the real and imaginary parts separately. If we have a sorted array, for example, then clipping typically changes the array to have three regions: [min_value, unchanged, max_value]. If we clip the real and imaginary parts of a complex array separately, however, then we will no longer have these separable regions. Instead, values may change even if they don't compare less than the min or greater than the max!

I would also recommend disabling the behavior until there's confidence about what, exactly, clipping a complex number should do. My proposal would be that if c < min_value it's set to min_value, and if c > max_value its set to max_value.

@seberg
Copy link
Member

@seberg seberg commented Apr 10, 2020

@mruberry, I agreed, in any case I am not even sure anyone had even a use-case for this, which would go a long way to motivate me to put in anything. As one argument to make it a bit less bad, if you ensure that min.real <= max.real and min.imag <= max.imag as opposed to only min <= max, than things are fine (although we currently do not actually ensure this in NumPy).

@mruberry
Copy link

@mruberry mruberry commented Apr 10, 2020

I'm agreeing with you, too @seberg ;).

I think we're going to disable clipping (we call it clamping) complex inputs in PyTorch for now. Our behavior was inconsistent with NumPy's, anyway.

@anirudh2290
Copy link
Member

@anirudh2290 anirudh2290 commented May 27, 2020

just reading through this issue, i think that given the lexicographic comparison in numpy, this behavior actually makes sense. if we are deprecating lexicographic comparison though, clip, max and min should also follow. I am assuming that this was the consensus reached here, but I wasn't very sure from the comments and the linked PRs.

@jwprice100
Copy link

@jwprice100 jwprice100 commented Apr 1, 2021

So I just ran into an issue with clipping complex numbers. I'm not sure my use case is valid for the broader community but here it is.

I was trying to mimic the behavior of my hardware DSP chain. I use floating point to represent my hardware fixed point values. This works great as long as it's less than say 52 bits total. Part of the behavior is saturating as opposed to wrapping if I run out of bits to represent a value. So I clipped complex values like so:

np.clip(complex_array, -32768, 32767)

In the original array was a value of 32767+91j. After clipping it became32767+0j. I don't think that behavior is correct, or at least I couldn't imagine an interpretation that got me that result. I'm not sure what the right behavior is, but thought contributing my use case might help make that clear. What I expected for that value was no change, 32767+91j. If the real/imag components had been adjusted to scale magnitude, but keep phase the same, I would have understood that and noticed it much sooner. I'd also be ok if clip just rejected complex arrays.

It was easy enough to work around once I recognized what was happening, but I lost some time to what I perceived as non-intuitive behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants