-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay][Frontend] Implement SpaceToBatchND in Tensorflow frontend #2943
Conversation
@srkreddy1238 Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alexeyr for adding this op.
Can you add few test cases to non zero values for padding ?
You may add the test cases directly from https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/space-to-batch-n-d
too.
50116f0
to
558e246
Compare
@srkreddy1238 I've added the tests based on these examples and TF tests. I believe there is a typo in one of the TF examples: tensorflow/tensorflow#27638. |
@srkreddy1238 Actually, now that I know BatchToSpaceND exists, might as well add it too. I'll try to do it today. |
@srkreddy1238 Would you object to:
|
The main is only for local execution as CI runs with nose. Any welcome to make this change.
Name handling is good as it's important to make sure that Input and output names being untouched from original Tensorflow graph. Advice to leave it as it is for now. |
[[13], [14], [15], [16]]]], | ||
block_shape=[2, 2], | ||
paddings=[[0, 0], [0, 0]], | ||
desired=[[[[1], [3]], [[9], [11]]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you compute the desired from TF API instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've made this change already, just not pushed yet.
Ah, ok, then I don't need to worry about it. Still will probably do it just to make clear.
I think this could be handled entirely within the test and not exposed. But will leave for now, as you suggest. |
@srkreddy1238 Added BatchToSpaceND. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds a missing auxiliary operation used for dilated pooling and convolutions.