-
Notifications
You must be signed in to change notification settings - Fork 74k
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
Issue 4746 #4985
Issue 4746 #4985
Conversation
Can one of the admins verify this patch? |
@guotong1988, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vrv, @keveman and @zheng-xq to be potential reviewers. |
also test the code with:
|
@@ -448,7 +448,10 @@ Status AvgPoolShape(shape_inference::InferenceContext* c) { | |||
|
|||
Status MaxPoolShape(shape_inference::InferenceContext* c) { | |||
ShapeHandle input_shape; | |||
ShapeHandle kernel_sizes |
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.
Did you try building this? This looks like a syntax error.
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.
It's my mistake.I did build it but I make a copy mistake.
kernel_rows = c->Dim(kernel_sizes, 1); | ||
kernel_cols = c->Dim(kernel_sizes, 2); | ||
kernel_depth = c->Dim(kernel_sizes, 3); | ||
// kernel_depth = kernel_sizes[1]; |
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.
We need to clean these up.
Unfortunately, modifying an existing op like this breaks the backward compatiblity of TensorFlow. For now, it is a good idea to add a custom op on github outside TensorFlow, so people can try it if they are interested. When it gets popular and used in more models, we can move it to TF contrib or TF core. https://www.tensorflow.org/versions/r0.11/how_tos/adding_an_op/index.html Thanks. |
OK , I will take another issue. |
Thanks for looking into this! I'll close the PR. Feel free to reopen if appropriate. |
#4746
This code is the first commit to fix issue-4746 . I will commit the unit test code and doc later.
Thank you for your review . Thank you for any advice.I will work on it again for the advice and learn from the reviewers.
I test this code with: