Skip to content

Add support for integer dim arg in torch.linalg.norm #43907

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.

Sign up for GitHub

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

Jump to bottom
Closed
kurtamohler wants to merge 1 commit into from
Closed

Add support for integer dim arg in torch.linalg.norm #43907

kurtamohler wants to merge 1 commit into from

Conversation

@kurtamohler
Copy link
Collaborator

@kurtamohler kurtamohler commented Aug 31, 2020

Since PR #43262 is merged, this works now.

Part of #24802

@kurtamohler kurtamohler requested a review from mruberry August 31, 2020 21:17
@mattip mattip mentioned this pull request Aug 31, 2020
Closed
4 tasks
@dr-ci
Copy link

dr-ci bot commented Aug 31, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 32a79c6 (more details on the Dr. CI page):

  • 2/2 failures possibly* introduced in this PR
    • 2/2 non-CircleCI failure(s)

ci.pytorch.org: 2 failed

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.

See how this bot performed.

This comment has been revised 6 times.

@kurtamohler
Copy link
Collaborator Author

kurtamohler commented Aug 31, 2020

The pytorch_linux_backward_compatibility_check_test job failed, but that's unavoidable because I changed native_functions.yaml. The only actual BC breaking change is that calling torch.linalg.norm with dim being an int will now work correctly, instead of throwing an error.

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

mruberry commented Aug 31, 2020

The pytorch_linux_backward_compatibility_check_test job failed, but that's unavoidable because I changed native_functions.yaml. The only actual BC breaking change is that calling torch.linalg.norm with dim being an int will now work correctly, instead of throwing an error.

Breaking BC for torch.linalg.norm is also probably fine because it's so new no one has had a chance to use it. Just add an entry here:

https://github.com/pytorch/pytorch/blob/master/test/backward_compatibility/check_backward_compatibility.py

like for:

pytorch/test/backward_compatibility/check_backward_compatibility.py

Line 74 in 7680d87

("aten::linalg_outer", datetime.date(2020, 8, 30)),

with the operator + a date by which the PR will definitely be landed (I usually just put a month in). This will stop the backcompat build from complaining.

@codecov
Copy link

codecov bot commented Sep 1, 2020 •
edited
Loading

Codecov Report

Merging #43907 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #43907   +/-   ##
=======================================
  Coverage   69.24%   69.25%           
=======================================
  Files         378      378           
  Lines       46862    46862           
=======================================
+ Hits        32451    32452    +1     
+ Misses      14411    14410    -1
Impacted Files Coverage Δ
torch/testing/_internal/expecttest.py 78.57% <0.00%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7680d87...32a79c6. Read the comment docs.

@kurtamohler
Copy link
Collaborator Author

kurtamohler commented Sep 1, 2020

Doesn't seem like the pr/caffe2-pytorch-linux-xenial-rocm3.5.1-py3.6-test and pr/pytorch-linux-xenial-rocm3.5.1-py3.6 are related to my changes.

@mruberry
Copy link
Collaborator

mruberry commented Sep 2, 2020

Doesn't seem like the pr/caffe2-pytorch-linux-xenial-rocm3.5.1-py3.6-test and pr/pytorch-linux-xenial-rocm3.5.1-py3.6 are related to my changes.

Sorry about that, @kurtamohler, I agree those failures don't look related.

mruberry
mruberry approved these changes Sep 2, 2020
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Awesome improvement!

facebook-github-bot
facebook-github-bot reviewed Sep 2, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 6, 2020

@mruberry merged this pull request in 68297ee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@mruberry mruberry mruberry approved these changes

Assignees

No one assigned

Labels

Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kurtamohler @mruberry @facebook-github-bot @ailzhang @pytorchbot