Add functions add/sub/rsub/mul/div that accept a scalar as first arg#29602
Closed
xuhdev wants to merge 42 commits intogh/xuhdev/52/basefrom
Closed
Add functions add/sub/rsub/mul/div that accept a scalar as first arg#29602xuhdev wants to merge 42 commits intogh/xuhdev/52/basefrom
xuhdev wants to merge 42 commits intogh/xuhdev/52/basefrom
Conversation
[ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
ailzhang
requested changes
Nov 12, 2019
Contributor
ailzhang
left a comment
There was a problem hiding this comment.
Hmmmm this is a lot of overloads...
I'd take a step back and do the design carefully first.
There are two benefits of having this PR:
- Currently in C++ frontend
torch::add(1, tensor)doesn't compile. This PR will solve that. - This PR combines the next in the stack together will remove the hack in
python_args_parserwhere we convert a scalar directly to a tensor.
My main concern about this PR is: - Number of overloads added and the future maintenance burden with it.
cc: @ezyang @gchanan @yf225 to comment here.
(Sorry I'm not really requesting changes but to make sure we don't proceed without a design.
Contributor
|
I shouldn't make the call here, but my 2c is that for binary ops, it seems acceptable to me to have |
Contributor
|
I think this is an important addition for Python/C++ API parity, otherwise there will be friction and ongoing maintenance overhead if the user has to translate Python code |
Collaborator
|
Is there some other approach we can take with this, even if it's not an immediate change in this PR but something to think about for refactoring longer-term? I could imagine having functions be polymorphic in some way where we don't have to define any functions as accepting Scalar (other than in cases where they explicitly do not accept tensors). I'd think any function that can accept a tensor parameter should also be able to accept a scalar, and have it be interpreted as a zero-dim "wrapped number" tensor without extra handling or interfaces. It's odd to me that we can torch.pow(tensor, scalar), torch.pow(scalar,tensor), but not torch.pow(scalar, scalar). If we rely on implementing every combination manually, we'll always have some gaps. I know it's probably uncommon to want to use torch.pow() on two scalars, but consistency helps keep user code generic across inputs. I imagine there are at least a few ways we could solve for this. |
Collaborator
|
Also, I think these new interfaces would need to be handled in shape_analysis.cpp? |
Collaborator
Author
|
@nairbv We might be able to use some templating with some helpers, but I think that might likely also require some other changes in how function declarations are generated... |
Contributor
|
I mean, we can talk all we want about how exactly the C++ API should be implemented, but at the end of the day, we still need to actually implement kernels for each of the overloads. This isn't evident from this PR because it's still using the old mechanism ( |
gchanan
reviewed
Nov 15, 2019
gchanan
reviewed
Nov 15, 2019
gchanan
requested changes
Nov 15, 2019
| variants: function | ||
| supports_named_tensor: True | ||
|
|
||
| - func: rsub.Scalar2(Scalar other, Tensor self, Scalar alpha=1) -> Tensor |
Contributor
There was a problem hiding this comment.
do we need rsub? Why would I call this in C++? Is it necessary for the python binding later?
Collaborator
Author
There was a problem hiding this comment.
I don't think we need rsub (including the original ones), but we may need rsub_. I added merely because there are rsubs before. I can attempt to delete the existing rsubs if they are truly unecessary.
aten/src/ATen/native/BinaryOps.cpp
Outdated
| } | ||
|
|
||
| Tensor sub(Scalar other, const Tensor& self, Scalar alpha) { | ||
| return native::sub(wrapped_scalar_tensor(other), self, alpha); |
Contributor
There was a problem hiding this comment.
this is wrong, same as add.
Collaborator
Author
There was a problem hiding this comment.
I think this is correct? It is different from add.
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
Contributor
|
Removing myself from reviewer list. If there's something specific you want me to look at let me know. |
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
Member
💊 CircleCI build failures summary and remediationsAs of commit 4a47c6c (more details on the Dr. CI page):
🕵️ 12 new failures recognized by patternsThe following build failures do not appear to be due to upstream breakages:
|
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
Closed
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
Contributor
|
@xuhdev are you still working on this or do you need another review round? |
Collaborator
Author
|
I'm still working on this (but slowly). I probably need some help, as it seems like a lot of JIT code is involved, which I'm unfamiliar with... |
Collaborator
Author
|
@ezyang I'm unlikely to work on this PR this week (like the past week), and I'm not sure about the week after this week. Do you have an urgent need of this PR? If so, I'm OK with letting someone else take over it. |
Contributor
|
Nope, not urgent, just trying to make sure people aren't blocked.' It looks like you are blocked, but I'm not sure if there's someone on our side with time to help you unblock, sorry :( |
Collaborator
Author
|
@ezyang That's awesome! Let me come back to this some time later. |
… first arg" Because #29238 removes special processing of `add/sub/rsub/mul/div`, calls like `torch.add(Scalar, Tensor)` will not be automatically translated to `torch.add(Tensor, Tensor)`. Therefore, we explicitly overload these ops with a scalar as their first arg. [ghstack-poisoned]
Contributor
|
Hi @xuhdev! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but we do not have a signature on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Contributor
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Collaborator
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
Contributor
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
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.
Stack from ghstack:
Because #29238 removes special processing of
add/sub/rsub/mul/div, calls liketorch.add(Scalar, Tensor)will not be automatically translated totorch.add(Tensor, Tensor). Therefore, we explicitly overload these ops with a scalar as their first arg.