Switch over to max_pool_v2 in Python #14983
Conversation
yongtang
force-pushed
the
11875-ksize-max-pool-v2-follow-up
branch
2 times, most recently
from
25ed43b
to
65f377f
Compare
yongtang
changed the title
| stride_x = strides_attr.list.i[2] | ||
| if node.op == "MaxPoolV2": | ||
| strides_input_name = node.input[2] | ||
| if not strides_input_name.endswith("/strides"): |
There was a problem hiding this comment.
Is this safe? Could there possible be multiple strides tensors in the innermost scope, e.g. strides_1?
There was a problem hiding this comment.
@rmlarsen I think it is fine. The name is node.input[2] and the op is checked with node.op == "MaxPoolV2".
| if not strides_input_name.endswith("/strides"): | ||
| raise ValueError("Strides name does not end with '/strides'") | ||
| strides_node = name_to_order_node[strides_input_name].node | ||
| if strides_node.op != "Const": |
There was a problem hiding this comment.
Why does it need to be Const? Isn't the point of the new version that it can take a computed tensor value as argument?
There was a problem hiding this comment.
@rmlarsen Removed. Thanks.
| if node.op == "MaxPoolV2": | ||
| ksize_input_name = node.input[1] | ||
| if not ksize_input_name.endswith("/ksize"): | ||
| raise ValueError("Kernel size name does not end with '/ksize'") |
There was a problem hiding this comment.
same concern about uniqueness here?
| raise ValueError("Kernel size name does not end with '/ksize'") | ||
| ksize_node = name_to_order_node[ksize_input_name].node | ||
| if ksize_node.op != "Const": | ||
| raise ValueError("Kernel size op is not Const") |
rmlarsen
added
stat:awaiting response
yongtang
force-pushed
the
11875-ksize-max-pool-v2-follow-up
branch
from
6115296
to
23fb065
Compare
|
Thanks @rmlarsen for the review. The PR has been updated. |
|
@rmlarsen WDYT? |
|
Nagging Awaiting Response: It has been 14 days with no activityand the |
|
@yongtang sorry about dropping this. Please resolve the conflicts. |
This fix is a follow up to 11875 so that MaxPool in Python use v2 version. As 11875 has been merged some time ago, this fix conforms to the deprecation policy. This fix is realted to 11875 and 4746. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
…> MaxPoolV2 Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Update tensorflow/contrib/receptive_field due to max_pool's strides and ksize from attr -> input Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
yongtang
force-pushed
the
11875-ksize-max-pool-v2-follow-up
branch
from
23fb065
to
5af65cb
Compare
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
yongtang
force-pushed
the
11875-ksize-max-pool-v2-follow-up
branch
from
5af65cb
to
3208084
Compare
|
@rmlarsen The PR has been rebased with conflict resolved. Please take a look. |
rmlarsen
added
awaiting testing (then merge)
kokoro:force-run
rmlarsen
added
kokoro:run
kokoro:force-run
|
@yongtang this appears to break some tests in tensorflow/compiler: https://source.cloud.google.com/results/invocations/cc0169ef-2973-4bee-839e-94ba7b4e043b/targets please take a look. |
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
rmlarsen
removed
the
stat:awaiting response
|
@yongtang we are going to temporarily revert this change for the upcoming release to make sure this is also supported by the MKL accelerated code when enabled. |
|
Thanks @rmlarsen. I didn't know it caused the issue in MKL. Let me know if I can be of any help with migrating MKL's max_pool to v2. |
This fix is a follow up to #11875 so that MaxPool in Python use v2 version. As 11875 has been merged several months ago, this fix conforms to the deprecation policy.
This fix is realted to #11875 and #4746.
Signed-off-by: Yong Tang yong.tang.github@outlook.com