Improve torch.norm functionality, errors, and tests
#41956
Conversation
💊 CI failures summary and remediationsAs of commit ab7546e (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 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. This comment has been revised 30 times. |
kurtamohler
force-pushed
the
norm-functionality-fixes-24802
branch
2 times, most recently
from
ba0fcdc to
50b7cca
Compare
kurtamohler
changed the title
torch.norm behaviortorch.norm functionality, errors, and tests
kurtamohler
force-pushed
the
norm-functionality-fixes-24802
branch
from
50b7cca to
933fcc9
Compare
ngimel
added
the
triaged
|
I'm pretty sure the |
It's not. Unfortunately that build's very flaky, sorry. Also, I hope to get a chance to look at this later today. Sorry to keep you waiting, @kurtamohler. |
|
No worries @mruberry ! |
mruberry
left a comment
mruberry
left a comment
There was a problem hiding this comment.
Hey @kurtamohler! This looks pretty good to me. I added a couple requests for comments and suggested some changes to the test for coverage and maintainability.
This will be BC-breaking for users who were using keepdims previously, right? Would you write a "BC-Breaking Note" section at the top of the PR summary describing the before/after? We use these notes to write the release docs.
* Fix keepdim behavior in nuclear_norm and norm * Throw descriptive errors for unsupported sparse norm args * Increase unit test coverage for these cases and for complex inputs
kurtamohler
force-pushed
the
norm-functionality-fixes-24802
branch
from
bc37b5f to
ab7546e
Compare
|
Thanks for the review @mruberry! I believe I've addressed all your suggestions now. Also added the BC breaking note in the description. |
mruberry
added
the
module: linear algebra
mruberry
left a comment
mruberry
left a comment
There was a problem hiding this comment.
Thanks @kurtamohler! Looks great.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
5 participants
BC-Breaking Note:
BC breaking changes in the case where keepdim=True. Before this change, when calling
torch.normwith keepdim=True and p='fro' or p=number, leaving all other optional arguments as their default values, the keepdim argument would be ignored. Also, any timetorch.normwas called with p='nuc' and keepdim=True, the result would have one fewer dimension than the input, and the dimensions could be out of order depending on which dimensions were being reduced. After the change, for each of these cases, the result has the same number and order of dimensions as the input.PR Summary:
These changes were taken from part of PR #40924. That PR is not going to be merged because it overrides
torch.norm's interface, which we want to avoid. But these improvements are still useful.Issue #24802