Skip to content

Conversation

cosw0t
Copy link

@cosw0t cosw0t commented Sep 16, 2020

Fixes #{issue number}

@dr-ci
Copy link

dr-ci bot commented Sep 16, 2020

🔗 Helpful links

💊 CI failures summary and remediations

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


Commit d04d7ab was recently pushed. Waiting for builds...


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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@mruberry mruberry requested review from kurtamohler and mruberry and removed request for apaszke September 17, 2020 03:18
@mruberry mruberry added module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 17, 2020
@cosw0t cosw0t force-pushed the linalg_norm branch 3 times, most recently from 14d4af2 to 575969c Compare September 17, 2020 13:56
Copy link
Collaborator

@kurtamohler kurtamohler left a comment

Choose a reason for hiding this comment

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

The p kwargs need to be updated to ord. Otherwise, looks good

@cosw0t cosw0t force-pushed the linalg_norm branch 7 times, most recently from 6429fe7 to 6e78a65 Compare September 24, 2020 10:49
@cosw0t
Copy link
Author

cosw0t commented Sep 24, 2020

still lots of failed tests. surely this couldn't have come from changing the "p" to "ord" as the underlying functionality is unchanged?

@cosw0t cosw0t force-pushed the linalg_norm branch 4 times, most recently from 5025eb8 to 2505a9e Compare October 1, 2020 08:50
@cosw0t cosw0t force-pushed the linalg_norm branch 2 times, most recently from c498038 to ed05651 Compare October 5, 2020 10:58
@kurtamohler
Copy link
Collaborator

kurtamohler commented Oct 5, 2020

still lots of failed tests. surely this couldn't have come from changing the "p" to "ord" as the underlying functionality is unchanged?

Sorry for the late reply, I wasn't getting notifications on this PR for some reason. The functionality had to be changed in some cases because torch.norm did not match numpy.linalg.norm. Most notably, when the reduction spans two dimensions and p is a number, torch.linalg.norm will use a matrix norm, whereas torch.norm uses a vector norm of the two dimensions flattened into one. For instance:

>>> torch.norm(a, p=2)
tensor(3.1754)
>>> torch.linalg.norm(a, ord=2)
tensor(2.9176)
>>> torch.linalg.norm(a.flatten(0, 1), ord=2)
tensor(3.1754)

Also, the matrix norm can only be calculated with a few possible values of ord, unlike the vector norm, which will accept any real number.

It looks like most of the test failures are due to the added matrix norm functionality, so flatten will most likely need to be used to get the correct behavior.

Comparing the documentation for torch.linalg.norm in torch/linalg/__init__.py with torch.norm's documentation should shed some light. If not, then we'll need to update the documentation.

@kurtamohler
Copy link
Collaborator

Perhaps it would be helpful if I add a short guide to torch.linalg.norm's documentation describing the most common changes people will need to make when they update their torch.norm calls?

@cosw0t
Copy link
Author

cosw0t commented Oct 7, 2020

When the 2d norm differs I suppose the reference value should be changed to match the matrix norm, not the test one right? I suppose using flatten on the test value would be testing vector norm instead matrix norm and therefore not be a useful test?

Also when matrix norm fails because the order doesn't make any sense (eg ord=0.5) would we then flatten just so that it works, or do we remove the test case? e.g.: https://github.com/pytorch/pytorch/blob/master/test/test_nn.py#L1715

Perhaps it would be helpful if I add a short guide to torch.linalg.norm's documentation describing the most common changes people will need to make when they update their torch.norm calls?

very short, yes please

@kurtamohler
Copy link
Collaborator

kurtamohler commented Oct 7, 2020

When the 2d norm differs I suppose the reference value should be changed to match the matrix norm, not the test one right? I suppose using flatten on the test value would be testing vector norm instead matrix norm and therefore not be a useful test?

No, I think we really do want vector norms in these cases. These test failures are coming from operations that were implemented with torch.norm underneath. We can't change the behavior of those operations to use matrix norm instead of vector norm, because they would produce a different result than they used to. That would be BC breaking, and perhaps just generally incorrect in some cases.

For instance, in the case of clip_grad_norm, the documentation explicitly says that it uses a vector norm: "norm is computed over all gradients together, as if they were concatenated into a single vector" (https://pytorch.org/docs/stable/generated/torch.nn.utils.clip_grad_norm_.html?highlight=clip_grad_norm#torch.nn.utils.clip_grad_norm_)

Also when matrix norm fails because the order doesn't make any sense (eg ord=0.5) would we then flatten just so that it works

Yes, flattening is the right way to go here.

I'll submit an issue and start writing a torch.norm to torch.linalg.norm porting guide.

@cosw0t
Copy link
Author

cosw0t commented Oct 7, 2020

thanks!

btw regarding the difference between the 2 functions and which norm they compute on 2 dims, I think the docs say that they basically do the exact same thing, am I wrong?

From: https://pytorch.org/docs/master/linalg.html?highlight=norm#torch.linalg.norm

If dim is an int, vector norm will be calculated over the specified dimension. If dim is a 2-tuple of ints, matrix norm will be calculated over the specified dimensions. If dim is None, matrix norm will be calculated when the input tensor has two dimensions, and vector norm will be calculated when the input tensor has one dimension.

From: https://pytorch.org/docs/master/generated/torch.norm.html?highlight=norm#torch.norm

If it is an int, vector norm will be calculated, if it is 2-tuple of ints, matrix norm will be calculated. If the value is None, matrix norm will be calculated when the input tensor only has two dimensions, vector norm will be calculated when the input tensor only has one dimension. If the input tensor has more than two dimensions, the vector norm will be applied to last dimension.

am I losing it? is the docs wrong?

@kurtamohler
Copy link
Collaborator

am I losing it? is the docs wrong?

You're right, the documentation is misleading. I think the "matrix norm" that torch.norm's documentation is talking about is semantically different than the actual mathematical matrix norm. Rather than a true matrix norm, it's referring to whatever the "matrix norm" column maps that term to in the table under the p parameter. In this case, "matrix norm" where p is neither 'fro' nor 'nuc' (those are true matrix norms), we actually map it to a vector norm. Yes that is confusing. Maybe I should change my PR related to torch.norm's documentation (#42696) to fix this inconsistency.

@cosw0t
Copy link
Author

cosw0t commented Oct 7, 2020

yes I can see that now, thanks! Regardless what the docs say, it's clear what the behaviour is, so I need to fix the tests.

@cosw0t
Copy link
Author

cosw0t commented Oct 9, 2020

what about the definition of torch.Tensor.norm ? https://pytorch.org/docs/master/tensors.html?highlight=norm#torch.Tensor.norm
is it still going to use torch.norm function as per documentation or linalg?

@mruberry
Copy link
Collaborator

mruberry commented Oct 9, 2020

what about the definition of torch.Tensor.norm ? https://pytorch.org/docs/master/tensors.html?highlight=norm#torch.Tensor.norm
is it still going to use torch.norm function as per documentation or linalg?

still torch.norm for the forseeable future.

@mruberry
Copy link
Collaborator

Closing due to lack of updates.

@mruberry mruberry closed this Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul 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.

5 participants