Skip to content

Add torch.flip{lr, ud}#38599

Closed
kshitij12345 wants to merge 13 commits intopytorch:masterfrom
kshitij12345:develop/numpy/fliplr-flipud
Closed

Add torch.flip{lr, ud}#38599
kshitij12345 wants to merge 13 commits intopytorch:masterfrom
kshitij12345:develop/numpy/fliplr-flipud

Conversation

@kshitij12345
Copy link
Copy Markdown
Collaborator

@kshitij12345 kshitij12345 commented May 16, 2020 •
edited
Loading

Reference: #38349

TODO:

  • Add Tests

@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 16, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

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

@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented May 16, 2020

@mruberry Please review.

Will add the test post #37826 as I want to use _rand_shape from it.

Thanks:).

@mruberry mruberry added the module: numpy Related to numpy support, and also numpy compatibility of our operators label May 17, 2020
@mruberry mruberry self-requested a review May 17, 2020 10:06
mruberry
mruberry reviewed May 18, 2020
View reviewed changes
Comment thread torch/_torch_docs.py Outdated
mruberry
mruberry reviewed May 18, 2020
View reviewed changes
Comment thread torch/_torch_docs.py Outdated
mruberry
mruberry reviewed May 18, 2020
View reviewed changes
Comment thread torch/_torch_docs.py Outdated
mruberry
mruberry reviewed May 18, 2020
View reviewed changes
Comment thread torch/_torch_docs.py Outdated
mruberry
mruberry requested changes May 18, 2020
View reviewed changes
Copy link
Copy Markdown
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.

This is great, thanks @kshitij12345! I made some comments to improve the documentation. Also, I'd like to add more testing. We should add test_fliplr and test_flipud to TestTorchDeviceType in test_torch.py. That will let us test them on CPU and CUDA and on multiple device types, too. Further, that test class defines a helper, _np_compare, that you can use to implement most of the test for you. It will compare the results of torch.flip{lr, ud} to NumPy's results. Take a look at some of the tests using _np_compare already and let me know if you have any questions.

Once the docs are improved and we get the new tests in I think this PR will be good to go!

@kshitij12345
address comment
8bc00bd 
* update docs.
@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented May 18, 2020

@mruberry

  • Have updated the docs.

  • For adding test to TestTorchDeviceType, I am actually waiting for support flip and rot90 for complex dtype #37826 (which I feel is almost ready) as I am planning to use _rand_shape and _np_compare_func (which copies the result np array. This is needed as we are flipping the arrays here which leads to negative strides).

@anjali411 anjali411 self-requested a review May 18, 2020 18:57
mruberry
mruberry reviewed May 19, 2020
View reviewed changes
Comment thread torch/_torch_docs.py Outdated
mruberry
mruberry reviewed May 19, 2020
View reviewed changes
Comment thread torch/_torch_docs.py Outdated
@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented May 19, 2020

@mruberry

  • Have updated the docs.
  • For adding test to TestTorchDeviceType, I am actually waiting for support flip and rot90 for complex dtype #37826 (which I feel is almost ready) as I am planning to use _rand_shape and _np_compare_func (which copies the result np array. This is needed as we are flipping the arrays here which leads to negative strides).

Sounds smart. Just re-request when you're ready. I pointed out a couple doc nits and @anjali411 has a suggestion for autograd testing, too.

@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 21, 2020
@kshitij12345 kshitij12345 mentioned this pull request Jun 3, 2020
Closed
@kshitij12345
Merge branch 'master' into develop/numpy/fliplr-flipud
1121cb1
@kshitij12345
add tests
8bf1fc9
@kshitij12345
address comment
700efe0 
* fix `note`
@kshitij12345
fixes
43e42ac 
* use correct function in flipud_invalid.
* use assertRaisesRegex.
* add comment for using _np_compare_func.
@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented Jun 3, 2020

@mruberry @anjali411

  • Have updated the docs.
  • Have added tests in test_torch.py.
  • Have added the functions in whitelist for complex autograd in test_autograd.py

Do review. Thanks.

mruberry
mruberry reviewed Jun 3, 2020
View reviewed changes
Comment thread test/test_torch.py Outdated
mruberry
mruberry reviewed Jun 3, 2020
View reviewed changes
Comment thread test/test_torch.py Outdated
mruberry
mruberry requested changes Jun 3, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Looks very good. Made a couple notes that we can reduce the test surface a little and should avoid a merge conflict by waiting a moment to rebase this PR. This PR also needs an update to:

Then we'll just need to verify the tests pass.

@kshitij12345
Merge branch 'master' of https://github.com/pytorch/pytorch into deve…
1ef1d52 
…lop/numpy/fliplr-flipud
@kshitij12345
address comments
9851a11 
* reduce test dtypes.
* use compare_with_numpy.
* update docs rst file.
* add entry to aten_interned_strings.
@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented Jun 4, 2020

@mruberry
Have addressed the comments. PTAL:)

@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented Jun 4, 2020

Flake8 failure looks non-relevant to the PR.

@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented Jun 4, 2020

Flake8 failure looks non-relevant to the PR.

You're correct. I submitted a fix but you have to rebase to see it.

@mruberry mruberry self-requested a review June 5, 2020 05:11
@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented Jun 8, 2020

Gentle ping:)

mruberry
mruberry approved these changes Jun 8, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Awesome! Thanks @kshitij12345!

If you'd like to implement another NumPy function that Torch is missing, maybe maximum and minimum or broadcast_to?

facebook-github-bot
facebook-github-bot reviewed Jun 8, 2020
View reviewed changes
Copy link
Copy Markdown
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.

@kshitij12345 kshitij12345 deleted the develop/numpy/fliplr-flipud branch June 9, 2020 15:09
@kshitij12345
Copy link
Copy Markdown
Collaborator Author

kshitij12345 commented Jun 9, 2020

Thanks.

Sure. Would love to try maximum and minimum.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jun 9, 2020

@mruberry merged this pull request in 9733390.

@mruberry mruberry mentioned this pull request Jun 15, 2020
Closed
41 tasks
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@kshitij12345 @facebook-github-bot
Add torch.flip{lr, ud} (pytorch#38599)
beb54fa 
Summary:
Reference: pytorch#38349

TODO:
* [x] Add Tests
Pull Request resolved: pytorch#38599

Differential Revision: D21941884

Pulled By: mruberry

fbshipit-source-id: 7a442ff11051c2c868cf8e3c04e4bba0f1a1d426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mruberry mruberry mruberry approved these changes

@anjali411 anjali411 Awaiting requested review from anjali411

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: numpy Related to numpy support, and also numpy compatibility of our operators 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.

5 participants

@kshitij12345 @mruberry @facebook-github-bot @ailzhang @pytorchbot