Skip to content

Improve doc for torch.add and add doc for torch.sub.#25114

Closed
xuhdev wants to merge 13 commits intogh/xuhdev/29/basefrom
gh/xuhdev/29/head
Closed

Improve doc for torch.add and add doc for torch.sub.#25114
xuhdev wants to merge 13 commits intogh/xuhdev/29/basefrom
gh/xuhdev/29/head

Conversation

@xuhdev
Copy link
Collaborator

@xuhdev xuhdev commented Aug 23, 2019

Stack from ghstack:

Pull Request resolved: #25114

@pytorchbot pytorchbot added the module: docs Related to our documentation, both in docs/ and docblocks label Aug 23, 2019
@xuhdev xuhdev requested review from colesbury, gchanan and izdeby August 23, 2019 19:48
Copy link
Contributor

@gchanan gchanan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't document this, it's considered deprecated, see https://github.com/pytorch/pytorch/blob/master/tools/autograd/deprecated.yaml.

@xuhdev
Copy link
Collaborator Author

xuhdev commented Aug 23, 2019

Don't document this, it's considered deprecated, see https://github.com/pytorch/pytorch/blob/master/tools/autograd/deprecated.yaml.

@gchanan I'm a bit confused. What is the replacement? How would one now, for example, do the fma operation (as add is also deprecated)?

@gchanan
Copy link
Contributor

gchanan commented Aug 26, 2019

Doesn't:

sub.Tensor(Tensor self, Tensor other, *, Scalar alpha=1) -> Tensor

work?

@xuhdev
Copy link
Collaborator Author

xuhdev commented Aug 26, 2019

@gchanan I see what you meant now -- you meant not to document the first variant, is that correct? I've removed the first variant from the doc.

@xuhdev xuhdev requested a review from gchanan August 26, 2019 18:47
@xuhdev xuhdev changed the title Add doc for torch.sub. Improve doc for torch.add and add doc for torch.sub. Aug 27, 2019
@xuhdev xuhdev requested a review from gchanan August 27, 2019 19:43
@xuhdev xuhdev requested a review from gchanan August 27, 2019 20:30
@xuhdev xuhdev requested a review from gchanan August 28, 2019 05:32
@xuhdev xuhdev requested a review from vishwakftw August 28, 2019 05:32
@xuhdev xuhdev requested a review from gchanan August 29, 2019 18:47
If :attr:`other` is of type FloatTensor or DoubleTensor, :attr:`alpha` must be
a real number, otherwise it should be an integer.
The category (floating point, integer, or Boolean) of :attr:`alpha` must match
that of the result of addition between :attr:`input` and :attr:`other`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't true. the following (integer alpha, float result) works for me on master:

import torch

x = torch.randn(3, 3)
y = torch.randn(3, 3)
torch.add(x, y, alpha=1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this to say the following:

If the result of the addition between :attr:`input` and :attr:`other` is:
- floating point, :attr:`alpha` can be `float` or `int`
- integral, :attr:`alpha` must be `int`
- boolean, :attr:`alpha` can be `int` or `bool.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I reworded a bit and updated this PR.

Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the category (floating point, integer, or Boolean) of :attr:alpha must match
a real number, otherwise it should be an integer. that of the result of addition between :attr:input and :attr:other." is wrong, let's reword it.

@xuhdev xuhdev requested a review from zou3519 October 7, 2019 19:36
@cpuhrsch cpuhrsch added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 11, 2019
@gchanan
Copy link
Contributor

gchanan commented Oct 15, 2019

@xuhdev should this be closed now that @nairbv's PR is in?

@xuhdev
Copy link
Collaborator Author

xuhdev commented Oct 15, 2019

@gchanan I don't think so -- @nairv's PR is for the 1.3 branch, but this is for master

@xuhdev
Copy link
Collaborator Author

xuhdev commented Oct 15, 2019

Of course, if you can cherry-pick the add and sub part of #27501 into master, I think this can be closed

@nairbv
Copy link
Collaborator

nairbv commented Oct 16, 2019

Of course, if you can cherry-pick the add and sub part of #27501 into master, I think this can be closed

My PR on the 1.3 branch conflicts with master, so probably easier to land this one than to cherry pick that one.

@xuhdev
Copy link
Collaborator Author

xuhdev commented Oct 16, 2019

Closed in favor of #28118, which also adds doc for mul and updates format starting from #27571

@xuhdev xuhdev closed this Oct 16, 2019
xuhdev added a commit to xuhdev/pytorch that referenced this pull request Oct 21, 2019
@facebook-github-bot facebook-github-bot deleted the gh/xuhdev/29/head branch November 16, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: docs Related to our documentation, both in docs/ and docblocks open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants