Skip to content

Conversation

kurtamohler
Copy link
Collaborator

@kurtamohler kurtamohler commented Aug 6, 2020

  • Removes incorrect statement that "the vector norm will be applied to the last dimension".
  • More clearly describe each different combination of p, ord, and input size.
  • Moves norm tests from test/test_torch.py to test/test_linalg.py
  • Adds test ensuring that p='fro' and p=2 give same results for mutually valid inputs

Fixes #41388

@dr-ci
Copy link

dr-ci bot commented Aug 6, 2020

💊 CI failures summary and remediations

As of commit 210dd6d (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


1 failure confirmed as flaky and can be ignored:

  • pytorch_macos_10_13_py3_test

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 27 times.

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

ezyang commented Aug 7, 2020

@WANGZhaowei-Wesley could you help take a look at this fix? Thanks!

@kurtamohler
Copy link
Collaborator Author

This might be nice to have merged before the next release. Although, we now have a warning on torch.norm that deprecates it in favor of torch.linalg.norm, so perhaps it's moot now.

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.

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

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #42696      +/-   ##
==========================================
- Coverage   68.19%   68.19%   -0.01%     
==========================================
  Files         410      410              
  Lines       53403    53403              
==========================================
- Hits        36418    36417       -1     
- Misses      16985    16986       +1     
Impacted Files Coverage Δ
torch/functional.py 88.21% <ø> (ø)
torch/testing/_internal/expecttest.py 77.55% <0.00%> (-1.03%) ⬇️

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 9fb8e33...210dd6d. Read the comment docs.

@mruberry
Copy link
Collaborator

mruberry commented Oct 7, 2020

@kurtamohler It looks like this was never merged and #41388 never closed. Is this still relevant? Should I take a look for you?

@kurtamohler
Copy link
Collaborator Author

@mruberry, thanks for asking, yes I think this is still relevant since torch.norm is still available.

@kurtamohler
Copy link
Collaborator Author

@mruberry, maybe hold off on reviewing for a bit. I want to see if I can fix some of the confusing points brought up here: #44796 (comment)

@mruberry
Copy link
Collaborator

mruberry commented Oct 7, 2020

@mruberry, maybe hold off on reviewing for a bit. I want to see if I can fix some of the confusing points brought up here: #44796 (comment)

Sounds great. We'll wait for your update.

@mruberry mruberry requested a review from ezyang October 7, 2020 17:08
@kurtamohler kurtamohler changed the title Fix mistake in norm documentation Fix mistakes and increase clarity of norm documentation Oct 7, 2020
@kurtamohler
Copy link
Collaborator Author

@mruberry, OK I think this is a bit better, ready for review.

@kurtamohler kurtamohler force-pushed the norm-doc-fix branch 4 times, most recently from 72e2a5f to 144b9ff Compare October 9, 2020 01:14
@mruberry mruberry self-requested a review October 9, 2020 02:51
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.

I think I finally understand what torch.norm does. Thanks @kurtamohler. Just ping me when the test(s) are move to test_linalg.py.

@kurtamohler
Copy link
Collaborator Author

I think I finally understand what torch.norm does.

Haha yes, me too, even though I've been working with it for a couple months now.

@kurtamohler
Copy link
Collaborator Author

kurtamohler commented Oct 9, 2020

@mruberry , I've moved the tests, should be good to go now.
EDIT: actually, getting some errors, just a minute
Fixed

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

@mruberry merged this pull request in a0a8bc8.

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

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

Development

Successfully merging this pull request may close these issues.

It seems the documentation of torch.norm is a little wrong.

6 participants