Skip to content

add fill_diagonal function #21892

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
shihongzhi wants to merge 13 commits into from
Closed

add fill_diagonal function #21892

shihongzhi wants to merge 13 commits into from

Conversation

@shihongzhi
Copy link
Contributor

@shihongzhi shihongzhi commented Jun 18, 2019

Fixes #21796

@pytorchbot pytorchbot added module: internals Related to internal abstractions in c10 and ATen module: operators labels Jun 18, 2019
colesbury
colesbury requested changes Jun 18, 2019
View reviewed changes
Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this issue! This needs a few changes before it can be merged:

  1. The API should match the NumPy API: https://docs.scipy.org/doc/numpy/reference/generated/numpy.fill_diagonal.html
  2. The function needs API documentation (in torch/_torch_docs.py)
  3. The function needs unit tests. (In test/test_torch.py)

Once the PR is ready for review, please click "dismiss review" on this

ssnl
ssnl reviewed Jun 18, 2019
View reviewed changes
@shihongzhi
1. match Numpy API
377c5b4 
2. add API doc
3. add unit tests
@pytorchbot pytorchbot added the module: docs Related to our documentation, both in docs/ and docblocks label Jun 19, 2019
@apaszke
Copy link
Contributor

apaszke commented Jun 22, 2019

To add to what @colesbury said, the name will also need to be changed to have a trailing underscore, since it modifies the input tensor.

@shihongzhi
Copy link
Contributor Author

shihongzhi commented Jun 24, 2019

@apaszke Already add trailing underscore to function name. Thank you

@colesbury I had add docs&unit tests and match this API to Numpy API

colesbury
colesbury reviewed Jun 24, 2019
View reviewed changes
Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Nice! Looking pretty good

@soumith soumith added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 25, 2019
@shihongzhi
1. add non-contiguous tensor unit test
1a8764f 
2. add large dim unit test
3. remove function
gchanan
gchanan reviewed Jun 25, 2019
View reviewed changes
@shihongzhi
Copy link
Contributor Author

shihongzhi commented Jun 27, 2019

@colesbury I fixed those bug. It's ready for review again.

colesbury
colesbury requested changes Jun 27, 2019
View reviewed changes
Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Some of the continuous builds are failing (test_torch (__main__.TestDocCoverage)) I think because of the function listed in torch.rst but it's not in the torch package.

Otherwise looks good to go

@shihongzhi
Copy link
Contributor Author

shihongzhi commented Jun 28, 2019

@colesbury Thank you. I removed fill_diagonal_ in torch.rst

@gchanan
Copy link
Contributor

gchanan commented Jul 2, 2019

@pytorchbot rebase this please.

@pytorchbot
Copy link
Collaborator

pytorchbot commented Jul 5, 2019

There's nothing to do! This branch is already up to date with master (d2ceab2).

(To learn more about this bot, see Bot commands.)

@shihongzhi
Copy link
Contributor Author

shihongzhi commented Jul 6, 2019

@colesbury I think this's ready for merge

facebook-github-bot
facebook-github-bot reviewed Jul 9, 2019
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.

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

@colesbury
Copy link
Member

colesbury commented Jul 9, 2019

Thanks @shihongzhi!

@xuhdev
Copy link
Collaborator

xuhdev commented Jul 11, 2019

Can we move the definition to a different file? It seems to me that fill_diagonal is not a factory. Perhaps create Fill.cpp and move both fill_diagonal and fill to it? I don't think this is urgent for this PR -- we can do this after this PR is merged.

@colesbury
Copy link
Member

colesbury commented Jul 11, 2019

@xuhdev yes, that seems reasonable. Do you want to make the change after this lands?

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 11, 2019
@shihongzhi @facebook-github-bot
add fill_diagonal function (#21892)
2b5cc8f 
Summary:
Fixes pytorch/pytorch#21796
Pull Request resolved: pytorch/pytorch#21892

Differential Revision: D16164678

Pulled By: colesbury

fbshipit-source-id: 85df8ae9b7a6a91b6023fe7295b3a8124e4526ea
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 11, 2019

@colesbury merged this pull request in 45cf33a.

@xuhdev
Copy link
Collaborator

xuhdev commented Jul 11, 2019

@colesbury See #22758

@izdeby izdeby mentioned this pull request Jul 11, 2019
Closed
izdeby added a commit that referenced this pull request Jul 11, 2019
@izdeby
add fill_diagonal function (#21892)
0924d8d 
Summary:
Fixes #21796
Pull Request resolved: #21892

Differential Revision: D16164678

Pulled By: colesbury

fbshipit-source-id: 85df8ae9b7a6a91b6023fe7295b3a8124e4526ea
@izdeby izdeby mentioned this pull request Jul 11, 2019
Closed
izdeby added a commit that referenced this pull request Jul 11, 2019
@izdeby
Update on "add fill_diagonal function (#21892)"
1ee4430 
Enable Bfloat16 tests in test_torch for cuda

fbshipit-source-id: 85df8ae9b7a6a91b6023fe7295b3a8124e4526ea

gh-metadata: pytorch pytorch 22772 gh/izdeby/16/head
@izdeby izdeby mentioned this pull request Jul 11, 2019
Closed
izdeby added a commit that referenced this pull request Jul 11, 2019
@izdeby
Update on "add fill_diagonal function (#21892)"
7ed296e 
Enable Bfloat16 tests in test_torch for cuda

fbshipit-source-id: 85df8ae9b7a6a91b6023fe7295b3a8124e4526ea

gh-metadata: pytorch pytorch 22772 gh/izdeby/16/head
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gchanan gchanan gchanan left review comments

@ssnl ssnl ssnl left review comments

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

@colesbury colesbury colesbury approved these changes

+1 more reviewer

@apaszke apaszke apaszke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen 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.

Add torch.fill_diagonal