Skip to content

Implementing Matrix Norm for torch.norm#11261

Closed
yya007-zz wants to merge 1 commit intopytorch:masterfrom
yya007-zz:myfeature
Closed

Implementing Matrix Norm for torch.norm#11261
yya007-zz wants to merge 1 commit intopytorch:masterfrom
yya007-zz:myfeature

Conversation

@yya007-zz
Copy link

@yya007-zz yya007-zz commented Sep 5, 2018

Currently, norm function only supports vector norm. This PR extends vector norm to matrix norm.

@yya007-zz
Copy link
Author

yya007-zz commented Sep 5, 2018 •
edited
Loading

This PR applied the comments in the # 10722 and redesigned the norm function. The internal reviewer could check T28601904 for details.

facebook-github-bot
facebook-github-bot reviewed Sep 5, 2018
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.

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

@ssnl ssnl self-requested a review September 5, 2018 23:23
ssnl
ssnl reviewed Sep 6, 2018
View reviewed changes
@ezyang
Copy link
Contributor

ezyang commented Sep 9, 2018

CC @erikbrinkman

@ezyang
Copy link
Contributor

ezyang commented Sep 10, 2018

I'm a bit uncertain if this is the way we want to support parameter-based choice of norm. @ssnl, you've thought about this before; should we start supporting strings for selecting this sort of thing? Should we block this until enums work?

@ssnl
Copy link
Collaborator

ssnl commented Sep 10, 2018 •
edited
Loading

@ezyang Yes enum support would be ideal. However, let's not block this bootcamp task for that, because afaik no one is actively working on that. Correct me if I am wrong though! :)

ssnl
ssnl reviewed Sep 10, 2018
View reviewed changes
Copy link
Collaborator

@ssnl ssnl left a comment

Choose a reason for hiding this comment

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

Can you also add relevant test to test_autograd to make sure that they are differentiable?

torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ssnl
ssnl reviewed Sep 11, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

@yya007-zz yya007-zz closed this Sep 14, 2018
@yya007-zz yya007-zz reopened this Sep 14, 2018
@yya007-zz
Copy link
Author

yya007-zz commented Sep 14, 2018 •
edited
Loading

@ssnl Autograd test was added.

ssnl
ssnl reviewed Sep 14, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

li-roy
li-roy approved these changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@li-roy li-roy left a comment

Choose a reason for hiding this comment

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

I feel like this is good to go.
Imo, we should accept the logic in python as an intermediate step, but since @ssnl has given more thought into this, I'll defer to you for the final decision.

li-roy
li-roy reviewed Sep 14, 2018
View reviewed changes
li-roy
li-roy suggested changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@li-roy li-roy left a comment

Choose a reason for hiding this comment

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

please resolve comments

@yya007-zz
Copy link
Author

yya007-zz commented Sep 14, 2018

@li-roy Thank you for the comments. I have resolved those.

ezyang
ezyang reviewed Sep 18, 2018
View reviewed changes
facebook-github-bot
facebook-github-bot reviewed Sep 18, 2018
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.

ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Sep 18, 2018
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.

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

@ssnl
Copy link
Collaborator

ssnl commented Sep 18, 2018

@pytorchbot retest this please

@yya007-zz yya007-zz force-pushed the myfeature branch 2 times, most recently from 242637a to 6feaf2a Compare September 19, 2018 01:13
@yya007-zz
Copy link
Author

yya007-zz commented Sep 19, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Sep 19, 2018
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.

li-roy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Sep 19, 2018
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.

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request Sep 20, 2018
@yya007 @facebook-github-bot
Implementing Matrix Norm for torch.norm (#11261)
cb89165 
Summary:
Currently, norm function only supports vector norm. This PR extends vector norm to matrix norm.
Pull Request resolved: pytorch/pytorch#11261

Reviewed By: li-roy

Differential Revision: D9652379

Pulled By: yya007

fbshipit-source-id: 519b3fb80b563c17c56a24675c7b0e46bf5a3a1c
@driazati driazati mentioned this pull request Sep 21, 2018
Closed
colesbury pushed a commit to colesbury/pytorch that referenced this pull request Sep 21, 2018
@facebook-github-bot
Remove tests from EXCLUDE_SCRIPT that pass (pytorch#11916)
4e65fbf 
Summary:
Spruriously added in pytorch#11261

I had a PR to catch these automatically (pytorch#11279), but it had some issues
passing on some CI environments but not others (e.g. for
`test_nn_group_norm`), any ideas?
Pull Request resolved: pytorch#11916

Differential Revision: D9992065

Pulled By: driazati

fbshipit-source-id: 05cfa8ed9af939e8ffd5827847ee7bfe0be799b2
@ezyang ezyang added the merged label Jun 26, 2019
@albanD albanD mentioned this pull request Apr 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang left review comments

@ssnl ssnl ssnl left review comments

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

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

@anderspapitto anderspapitto Awaiting requested review from anderspapitto

@bddppq bddppq Awaiting requested review from bddppq

@dzhulgakov dzhulgakov Awaiting requested review from dzhulgakov

@houseroad houseroad Awaiting requested review from houseroad

@jamesr66a jamesr66a Awaiting requested review from jamesr66a

@smessmer smessmer Awaiting requested review from smessmer

@Yangqing Yangqing Awaiting requested review from Yangqing

+2 more reviewers

@erikbrinkman erikbrinkman erikbrinkman left review comments

@li-roy li-roy li-roy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yya007-zz @ezyang @ssnl @erikbrinkman @facebook-github-bot @li-roy @yya007