Implement copysign #46396
Closed
Implement copysign #46396
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 310bc5e (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 2 times. |
zou3519
reviewed
Oct 15, 2020
💊 CI failures summary and remediationsAs of commit e3e5eed (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 2 failures confirmed as flaky and can be ignored:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 216 times. |
[numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: bool, int, short, long, float, double, half - Not available: byte, char, float/double complex TODO: - [ ] test - [ ] doc - [ ] kernel_vec [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 15, 2020
ghstack-source-id: ad138297abbe2e65bfb661c8446356e003ab152c Pull Request resolved: #46396
albanD
reviewed
Oct 15, 2020
| @@ -49,6 +49,11 @@ void copy_range(variable_list& out, IndexRange range, at::ArrayRef<Tensor> t) { | |||
| std::copy(t.begin(), t.end(), out.begin() + range.first); | |||
| } | |||
|
|
|||
| Tensor copysign_tensor_backward(Tensor grad, Tensor self, Tensor other) { | |||
| auto result = grad * self.sign() * other.sign(); | |||
There was a problem hiding this comment.
I am not sure that you need the self.sign() here.
It will fail gradcheck when you add it to the list in common_method_invocation.py if the formula is wrong. But I think you will need to change this.
There was a problem hiding this comment.
For instance:
a = tensor(-1.)b = tensor(1.)c = torch.copysign(a, b) = tensor(1)
The derivative of a is -1 rather than b.sign() = 1. Any thought on that?
There was a problem hiding this comment.
Ho right, the derivative is -1 when we change and 1 otherwise so you need both! Agree with you.
Also there is a corner case at 0 here where sign() returns 0. What is copysign() doing for that? Is the backward formula good for this case as well?
|
From reading your table of outputs, is the following correct: If it is the case, then it makes the gradient computation easier to derive for special points. So basically all the "?" in your table above should be 0 as they all correspond to this case where a~=0. |
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: bool, int, short, long, float, double, half - Integral promoted to float - Not available: byte, char, float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 1? | | 0 | -1 | -0 | -1? | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | -1? | | 0 | -0 | 0 | 1? | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | -1? | | 0 | 0 | 0 | 1? | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | -1? | | 0 | 1 | 0 | 1? | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [ ] test - [ ] doc - [x] ~kernel_vec~ [ghstack-poisoned]
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: bool, int, short, long, float, double, half - Integral promoted to float - Not available: byte, char, float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 1? | | 0 | -1 | -0 | -1? | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | -1? | | 0 | -0 | 0 | 1? | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | -1? | | 0 | 0 | 0 | 1? | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | -1? | | 0 | 1 | 0 | 1? | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [ ] test - [ ] doc - [x] ~kernel_vec~ [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 15, 2020
ghstack-source-id: 0378575506b0ec7464690dbb1b3ca4f7e8569224 Pull Request resolved: #46396
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: bool, int, short, long, float, double, half - Integral promoted to float - Not available: byte, char, float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 1? | | 0 | -1 | -0 | -1? | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | -1? | | 0 | -0 | 0 | 1? | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | -1? | | 0 | 0 | 0 | 1? | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | -1? | | 0 | 1 | 0 | 1? | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [ ] test - [ ] doc - [x] ~kernel_vec~ [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 16, 2020
ghstack-source-id: 2937708298c613db16a5b4fc2dfbd9cd45423fec Pull Request resolved: #46396
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: bool, int, short, long, float, double, half - Integral promoted to float - Not available: byte, char, float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | 0 | | 0 | -0 | 0 | 0 | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test - [ ] doc - [x] ~kernel_vec~ [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 16, 2020
ghstack-source-id: 5f477aa5d724aeaa1606d1aa690cf4778f433cec Pull Request resolved: #46396
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | 0 | | 0 | -0 | 0 | 0 | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test - [x] doc - [x] ~kernel_vec~ [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 16, 2020
ghstack-source-id: edccfde9095eab988ba254c001b784021fa6fe87 Pull Request resolved: #46396
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | 0 | | 0 | -0 | 0 | 0 | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test - [x] doc - [x] ~kernel_vec~ [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 19, 2020
ghstack-source-id: 0d5fc918521ebe7ac4e537eb9b13a2e3c18c2091 Pull Request resolved: #46396
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | 0 | | 0 | -0 | 0 | 0 | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test - [x] doc - [x] ~kernel_vec~ - [ ] torch.copysign(Number input, Tensor other) [ghstack-poisoned]
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | 0 | | 0 | -0 | 0 | 0 | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test - [x] doc - [x] ~kernel_vec~ - [ ] torch.copysign(Number input, Tensor other) [ghstack-poisoned]
Related #38349 [numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | 1 | -1 | | -0 | -0 | 0 | 0 | | 0 | -0 | 0 | 0 | | 1 | -0 | 1 | 1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test - [x] doc - [x] ~kernel_vec~ - [ ] torch.copysign(Number input, Tensor other) [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Oct 29, 2020
ghstack-source-id: c1a5bac31d691283301d70fa185e689f923b090a Pull Request resolved: #46396
mruberry
reviewed
Oct 30, 2020
mruberry
reviewed
Oct 30, 2020
mruberry
reviewed
Oct 30, 2020
Sounds great.
Just test if the second dtype is a float type or not and only perform that part of the test if it is. |
zou3519
reviewed
Nov 2, 2020
zou3519
reviewed
Nov 2, 2020
zou3519
reviewed
Nov 2, 2020
zou3519
reviewed
Nov 2, 2020
[numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | -1 | 1 | | -0 | -0 | -0 | 0 | | 0 | -0 | -0 | 0 | | 1 | -0 | -1 | -1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test (cpu/gpu) - [x] doc - [x] ~kernel_vec~ Differential Revision: [D24401366](https://our.internmc.facebook.com/intern/diff/D24401366) [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Nov 2, 2020
ghstack-source-id: 39d7768cbfba6c9c4eb5a090d0182b2423d9f990 Pull Request resolved: #46396
zou3519
reviewed
Nov 2, 2020
zou3519
reviewed
Nov 2, 2020
zou3519
reviewed
Nov 2, 2020
[numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | -1 | 1 | | -0 | -0 | -0 | 0 | | 0 | -0 | -0 | 0 | | 1 | -0 | -1 | -1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test (cpu/gpu) - [x] doc - [x] ~kernel_vec~ Differential Revision: [D24401366](https://our.internmc.facebook.com/intern/diff/D24401366) [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Nov 2, 2020
ghstack-source-id: caeb4463c61473183ac336f7d6eecb4fd05483c0 Pull Request resolved: #46396
[numpy](https://numpy.org/doc/stable/reference/generated/numpy.copysign.html?highlight=copysign#numpy.copysign) - No in-place function - No method - Optional output - Available: byte, char, bool, int, short, long, float, double, half - Integral promoted to float - Not available: float/double complex `c = np.copysign(a, b)` | a | b | c | a.grad | |:--:|:--:|:--:|:----:| | -1 | -1 | -1 | 1 | | -0 | -1 | -0 | 0 | | 0 | -1 | -0 | 0 | | 1 | -1 | -1 | -1 | | -1 | -0 | -1 | 1 | | -0 | -0 | -0 | 0 | | 0 | -0 | -0 | 0 | | 1 | -0 | -1 | -1 | | -1 | 0 | 1 | -1 | | -0 | 0 | 0 | 0 | | 0 | 0 | 0 | 0 | | 1 | 0 | 1 | 1 | | -1 | 1 | 1 | -1 | | -0 | 1 | 0 | 0 | | 0 | 1 | 0 | 0 | | 1 | 1 | 1 | 1 | This function becomes **non-differentiable** at `a=0` for any `b`. So, in my opinion, we may set the gradient for `a=0` to 0. TODO: - [x] test (cpu/gpu) - [x] doc - [x] ~kernel_vec~ Differential Revision: [D24401366](https://our.internmc.facebook.com/intern/diff/D24401366) [ghstack-poisoned]
ejguan
added a commit
that referenced
this pull request
Nov 3, 2020
ghstack-source-id: fdf7fbb840449a34cf23b09735eba05a1235fb19 Pull Request resolved: #46396
|
Update because of the following two reasons:
|
41 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #38349
Stack from ghstack:
numpy
c = np.copysign(a, b)This function becomes non-differentiable at
a=0for anyb. So, in my opinion, we may set the gradient fora=0to 0.TODO:
kernel_vecDifferential Revision: D24401366