Skip to content
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.

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

[Graph] Add Flip node. #3889

Closed
wants to merge 7 commits into from
Closed

Conversation

@mciprian13
Copy link
Contributor

mciprian13 commented Dec 11, 2019

Summary

  • Add Flip node inspired from Numpy.
  • The node reverts the order of the elements along a given axis.
  • The node currently supports single axis: multiple nodes can be chained for multiple axes.
  • The node can be used for example for RGB <-> BGR conversion inside the graph.

Documentation
None

Test Plan
Unit tests for operator, ONNX importer and exporter.

mciprian13 added 3 commits Dec 11, 2019
# Conflicts:
#	tests/unittests/OnnxImporterTest.cpp
Copy link
Contributor

jfix71 left a comment

Mostly LG -- couple comments.

// Reorder transformations
//===--------------------------------------------------------------------===//

BB.newInstr("Flip")

This comment has been minimized.

Copy link
@jfix71

jfix71 Dec 20, 2019

Contributor

Any reason we can't/shouldn't do this inplace?

This comment has been minimized.

Copy link
@mciprian13

mciprian13 Dec 20, 2019

Author Contributor

The implementations for both the Interpreter and the CPU do NOT allow for in-place computation (for in-place computation the implementation would be trickier).

This comment has been minimized.

Copy link
@jfix71

jfix71 Jan 9, 2020

Contributor

Right, I was suggesting we could change the implementations to be in place to reduce an unnecessary buffer, but we can land it as is/consider it for the future if desired.

tests/unittests/OperatorTest.cpp Outdated Show resolved Hide resolved
@mciprian13

This comment has been minimized.

Copy link
Contributor Author

mciprian13 commented Dec 24, 2019

Can we have this landed? Thanks!

mciprian13 added 2 commits Dec 30, 2019
@mciprian13

This comment has been minimized.

Copy link
Contributor Author

mciprian13 commented Jan 7, 2020

@jfix71 If there is nothing left to be modified, I think it is ready to be landed.
Thanks!

@jfix71
jfix71 approved these changes Jan 9, 2020
Copy link
Contributor

jfix71 left a comment

LGTM, sorry for the delay, thanks!

// Reorder transformations
//===--------------------------------------------------------------------===//

BB.newInstr("Flip")

This comment has been minimized.

Copy link
@jfix71

jfix71 Jan 9, 2020

Contributor

Right, I was suggesting we could change the implementations to be in place to reduce an unnecessary buffer, but we can land it as is/consider it for the future if desired.

Copy link

facebook-github-bot left a comment

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

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jan 11, 2020

@jfix71 merged this pull request in 2661e05.

@mciprian13 mciprian13 deleted the mciprian13:add_flip_node branch Jan 14, 2020
UncleGene added a commit to UncleGene/glow that referenced this pull request Jan 15, 2020
Summary:
**Summary**
- Add Flip node inspired from Numpy.
- The node reverts the order of the elements along a given axis.
- The node currently supports single axis: multiple nodes can be chained for multiple axes.
- The node can be used for example for RGB <-> BGR conversion inside the graph.

**Documentation**
None

**Test Plan**
Unit tests for operator, ONNX importer and exporter.
Pull Request resolved: pytorch#3889

Differential Revision: D19326679

Pulled By: jfix71

fbshipit-source-id: 103f0f84418e7197a937590c99347366897edd48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.