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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate torch.add(tensor, value, other) #19200

Open
zou3519 opened this issue Apr 12, 2019 · 3 comments
Open

Deprecate torch.add(tensor, value, other) #19200

zou3519 opened this issue Apr 12, 2019 · 3 comments
Labels
module: bc-breaking Related to a BC-breaking change module: deprecation module: numpy Related to numpy support, and also numpy compatibility of our operators module: ux triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@zou3519
Copy link
Contributor

zou3519 commented Apr 12, 2019

馃殌 Feature

It's confusing that add has the scale argument. Could we create a new torch.axpy function that does this?

Motivation

Numpy also makes this separation: they have numpy.add, and scipy.blas.axpy.
This could also make things nicer: add can be treated as an example of a binary pointwise operation like mul, sub, div and not require any special casing for its value argument.

cc @ezyang @gchanan @mruberry @rgommers @heitorschueroff @cpuhrsch

@vadimkantorov
Copy link
Contributor

@zou3519 Empirically, torch.add(torch.rand(3, 3), torch.rand(3, 3)) works, so it seems it does the dispatch correctly if it's used as a binary op. Strange.

@zou3519
Copy link
Contributor Author

zou3519 commented Apr 15, 2019

Another reason for deprecating it is that the overload cannot be written nicely with a single python function (without parsing *args and **kwargs).

The signature looks something like add(tensor, value=1, other): the value is optional.

@ezyang ezyang added module: operators module: bc-breaking Related to a BC-breaking change triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module enhancement Not as big of a feature, but technically not a bug. Should be easy to fix and removed triage review labels Apr 15, 2019
@ezyang
Copy link
Contributor

ezyang commented Apr 15, 2019

@colesbury says: it would be good to get rid of all of the weird ambiguous positional arguments, in general. (These cases may all be in deprecated.yaml).

@gchanan says: For example, sub, and all of the fused add ones, like addcmul. So we want a more general solution than just calling this axpy

@mruberry mruberry added module: ux and removed enhancement Not as big of a feature, but technically not a bug. Should be easy to fix module: operators (deprecated) labels Oct 8, 2020
@mruberry mruberry added module: deprecation module: numpy Related to numpy support, and also numpy compatibility of our operators labels Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: bc-breaking Related to a BC-breaking change module: deprecation module: numpy Related to numpy support, and also numpy compatibility of our operators module: ux 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

5 participants