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

torch.sign doesn't work for complex tensors #36323

Closed
anjali411 opened this issue Apr 9, 2020 · 18 comments
Closed

torch.sign doesn't work for complex tensors #36323

anjali411 opened this issue Apr 9, 2020 · 18 comments
Labels
module: complex Related to complex number support in PyTorch module: numpy Related to numpy support, and also numpy compatibility of our operators triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@anjali411
Copy link
Contributor

anjali411 commented Apr 9, 2020

Numpy:

>>> import numpy as np
>>> np.sign(np.array([1+2j]))
array([1.+0.j])

From numpy docs: For complex inputs, the sign function returns sign(x.real) + 0j if x.real != 0 else sign(x.imag) + 0j.
complex(nan, 0) is returned for complex nan inputs.

PyTorch

>>> import torch
>>> torch.sign(torch.tensor([1+2j]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: "sign_cpu" not implemented for 'ComplexFloat'

cc. @mruberry

cc @ezyang @anjali411 @dylanbespalko

@anjali411 anjali411 added the module: complex Related to complex number support in PyTorch label Apr 9, 2020
@mruberry mruberry added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Apr 9, 2020
@anjali411
Copy link
Contributor Author

anjali411 commented Apr 9, 2020

Although I think it makes more sense to return the sign for both real and imag though.
cc. @dylanbespalko

@zhangguanheng66 zhangguanheng66 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 9, 2020
@mruberry
Copy link
Collaborator

mruberry commented Apr 9, 2020

Although I think it makes more sense to return the sign for both real and imag though. like:
torch.tensor([1+2j, 1-2j, -1-2j]) should return torch.tensor([1+1j, 1-1j, -1-1j])
cc. @dylanbespalko

You're probably right that it would make more sense to do that, but because NumPy exists and is so popular it's already created a significant expectation of what sign() should do. Maybe the behavior you describe could be implemented with an optional kwarg or using a different function name?

@ezyang
Copy link
Contributor

ezyang commented Apr 10, 2020

@rgommers do you have an opinion on this matter?

@rgommers
Copy link
Collaborator

I don't think there's a clear choice here. Mathematically I'd probably expect x / |x|. Pragmatically I think here the two choices are to copy numpy, or error out so users choose a less ambiguous formulation in their code.

@mruberry
Copy link
Collaborator

"Yes, and..." to @rgommers.

If we're divergent from NumPy we should error. Wikipedia defines two sign functions for complex numbers: x / |x|, as @rgommers mentions, and csgn, which is what NumPy uses. If we don't think users have a natural expectation of one vs the other we could also implement both as something like sign_complex and csgn and error when a user tries to use sign alone on a complex tensor.

@rgommers
Copy link
Collaborator

we could also implement both as something like sign_complex and csgn and error when a user tries to use sign alone on a complex tensor.

That does feel like namespace pollution for something that won't be used a lot and is not difficult to implement by hand for a user in the way they want. I'm starting to lean more to just erroring out and leaving it at that.

@anjali411
Copy link
Contributor Author

anjali411 commented Apr 13, 2020

I don't think there's a clear choice here. Mathematically I'd probably expect x / |x|. Pragmatically I think here the two choices are to copy numpy, or error out so users choose a less ambiguous formulation in their code.

hmm. I remember @mmuckley mentioned that x / |x| is a commonly used and useful operation and it would be useful to have a method for it that gives a numerically stable version of it.

@mmuckley
Copy link

mmuckley commented Apr 13, 2020

Numpy has its own history discussing the matter:

numpy/numpy#3621

numpy/numpy#13179

As mentioned in the threads, most software packages other than Numpy use x/|x|, but the implementation in Numpy is a "valid analytic continuation of the real sign function." In MRI we never use this mathematical property, but we use x/|x| all the time. I can't speak to it in other fields. It looks like from the second issue that Numpy is considering two sign functions.

A few of us complex people are still in the process of coming over from Matlab where sign gives x/|x|, so the Numpy behavior was surprising to me.

@mruberry
Copy link
Collaborator

Since there are two valid implementations I think we have consensus to disable complex torch.sign for now to avoid user confusion. Thanks for your input, everyone!

@anjali411
Copy link
Contributor Author

synced with @mmuckley offline, and we think that adding a method that returns x / abs|x| is crucial for complex users. It can possibly be implemented under a different name like torch.sgn which btw numpy is also planning to add in future in addition to its existing torch.sign. A classic case for the use of x / abs|x| would be the iterative shrinkage algorithm for compressed sensing (Equation 1.5 here.

In addition I would also like to add that we should disable torch.sign since the current numpy behavior doesn't make much sense and we don't wanna diverge with numpy.

cc @mmuckley

@choidongyeon
Copy link

@anjali411 I would like to work on torch.sgn if it's still up for grabs.

@dylanbespalko
Copy link
Contributor

@anjali411,

x / abs|x| sounds like a really good idea to return a result that is on the unit circle. Great idea.

anjali411 added a commit that referenced this issue Jul 29, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

TODO:
1. add tests for backward (waiting on gradcheck PR for complex)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Aug 4, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

TODO:
1. add tests for backward (waiting on gradcheck PR for complex)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Aug 4, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Aug 4, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

[ghstack-poisoned]
@miccio-dk
Copy link

Hello everyone!
I installed pytorch-nightly using conda (from the pytorch-nightly repo) but I am still getting this error:
RuntimeError: "sign_cpu" not implemented for 'ComplexFloat'

I get the error while training a net that uses the following layer (which computes the response of an IIR filter from its
coefficients):

# iir filter  layer
class IIRFilter(torch.nn.Module):    
    def __init__(self, nfft):
        super(IIRFilter, self).__init__()
        self.nfft = nfft
    
    def forward(self, b, a):
        N = a.shape[0]
        bn = torch.cat([b, torch.zeros(N, self.nfft-3)], -1)
        an = torch.cat([a, torch.zeros(N, self.nfft-3)], -1)
        y = torch.view_as_complex(torch.rfft(bn, 1)) / torch.view_as_complex(torch.rfft(an, 1))
        y = y.abs()
        return y

I there otherwise a way I can implement the backward pass manually for the incriminating operations? (I am not interested in performances...yet)

Thanks in advance!

@anjali411
Copy link
Contributor Author

anjali411 commented Aug 10, 2020

Hello everyone!
I installed pytorch-nightly using conda (from the pytorch-nightly repo) but I am still getting this error:
RuntimeError: "sign_cpu" not implemented for 'ComplexFloat'

I get the error while training a net that uses the following layer (which computes the response of an IIR filter from its
coefficients):

# iir filter  layer
class IIRFilter(torch.nn.Module):    
    def __init__(self, nfft):
        super(IIRFilter, self).__init__()
        self.nfft = nfft
    
    def forward(self, b, a):
        N = a.shape[0]
        bn = torch.cat([b, torch.zeros(N, self.nfft-3)], -1)
        an = torch.cat([a, torch.zeros(N, self.nfft-3)], -1)
        y = torch.view_as_complex(torch.rfft(bn, 1)) / torch.view_as_complex(torch.rfft(an, 1))
        y = y.abs()
        return y

I there otherwise a way I can implement the backward pass manually for the incriminating operations? (I am not interested in performances...yet)

Thanks in advance!

The error is caused because torch.sign, which is used in the backward computation of torch.abs, is not defined for complex tensors. Complex Autograd is currently in a prototype state, and the backward functionality for a lot more functions will be included in the next release.

In the meantime, I think defining a backward function in the class IIRFilter and using that during training might work. cc. @albanD who works on Autograd.

@ezyang
Copy link
Contributor

ezyang commented Aug 10, 2020

If you want to manually implement, use the custom autograd api https://pytorch.org/docs/stable/notes/extending.html#extending-torch-autograd

@miccio-dk
Copy link

Alright, I'll check the docs on implementing the backward pass while waiting for the new release.
Thank you both for the help!

anjali411 added a commit that referenced this issue Aug 31, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Aug 31, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 1, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 3, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 3, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 21, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

This PR doesn't test the correctness of the gradients. It will be done as a part of auditing all the ops in future once we decide the autograd behavior (JAX vs TF) and add gradchek.

Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 21, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

also updates the backward definition of `torch.div`, `torch.abs`
Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 21, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

also updates the backward definition of `torch.div`, `torch.abs`
Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 21, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

also updates the backward definition of `torch.div`, `torch.abs`
Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 21, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

also updates the backward definition of `torch.div`, `torch.abs`
Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
anjali411 added a commit that referenced this issue Sep 22, 2020
resolves #36323 by adding `torch.sgn` for complex tensors.
`torch.sgn` returns `x/abs(x)` for `x != 0` and returns `0 + 0j` for `x==0`

also updates the backward definition of `torch.div`, `torch.abs`
Differential Revision: [D23460526](https://our.internmc.facebook.com/intern/diff/D23460526)

[ghstack-poisoned]
@MishinVD
Copy link

Alright, I'll check the docs on implementing the backward pass while waiting for the new release.
Thank you both for the help!
@miccio-dk
Good day! could you share how did you implement backward function in your case.?

@miccio-dk
Copy link

hey @MishinVD, see the code i was trying to get to work at #36323 (comment)

I'm currently using the nightly build of pytorch, which implements a new from torch.fft module.
Therefore, the new code is:

# iir filter response layer, similar to freqz
class IIRFilterResponse(torch.nn.Module):
    def __init__(self, nfft):
        super(IIRFilterResponse, self).__init__()
        self.nfft = nfft
        self.eps = torch.finfo(torch.float32).eps

    def forward(self, b, a):
        b_fft = rfft(b, n=self.nfft, dim=1)
        a_fft = rfft(a, n=self.nfft, dim=1)
        yabs = (torch.abs(b_fft) + self.eps) / (torch.abs(a_fft) + self.eps)
        assert torch.all(torch.isfinite(yabs))
        return yabs

Hope this helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: complex Related to complex number support in PyTorch module: numpy Related to numpy support, and also numpy compatibility of our operators triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

10 participants