Add Opset v10 support for MaxPool #488
Conversation
|
|
|
Please take a look at strict argument, https://github.com/onnx/onnx-tensorflow/blob/master/doc/CLI.md and see if it has any impact to the new version. |
|
@chinhuang007 the strict argument will not have influence on the v10 code as it will always generate semantically equivalent tensorflow model to the orignal model |
|
@tjingrant Please take a look since you have much better knowledge about the strict mode. Should we keep it and document that it has no effect in v10? or Merge pool() and pool_v10() which is time-consuming and might cause regression? |
There was a problem hiding this comment.
Sorry I'm only half way through, will continue tomorrow.
But let me post my comments now to expedite the review process.
Very nice implementation, but presentation can benefit from improvements.
Given that this is a tricky patch I'm probably going to be a bit more picky about the presentation than usual.
|
@tjingrant Thank you very much for your review. I'm working on addressing your comments and requests. I'll inform you when I have new version for review. Thanks! |
Version 10 support includes dilations and ceil_mode. This is new implementaion of dilated pooling by calculating and extracting (using tf.gather_nd) the values from the input Tensor that match the kernel, strides and dilations. It utilizes tf.nn.max_pool2d or tf.nn.max_pool_with_argmax to extract the final max values/indices. Also the code will utilize tf.nn.dilation2d when possible.
…axpool_with_argmax
- Added support for N-D max pooling with dilations - Cleaned up code and added comments - strict argument of the CLI is not processed as v10 max_pool will always generate semanticically equavalent tensorflow model to the orignal model.
- v10 auto_pad support had bugs - fixed the code to use auto_pad as a priority if both auto_pad and pads are specified - added unit tests for auto_pad
Version 10 support includes dilations and ceil_mode. This is new implementaion of dilated pooling by calculating and extracting (using tf.gather_nd) the values from the input Tensor that match the kernel, strides and dilations. It utilizes tf.nn.max_pool2d or tf.nn.max_pool_with_argmax to extract the final max values/indices. Also the code will utilize tf.nn.dilation2d when possible.
…axpool_with_argmax
- Added support for N-D max pooling with dilations - Cleaned up code and added comments - strict argument of the CLI is not processed as v10 max_pool will always generate semanticically equavalent tensorflow model to the orignal model.
- v10 auto_pad support had bugs - fixed the code to use auto_pad as a priority if both auto_pad and pads are specified - added unit tests for auto_pad
* moved few functions into pooling_helper and also created new tf_helper * made the code more readable and added comments with more detailed description of the algorithm used * few generalization changes to maxpool_mixin
only difference between v10 and v11 is that dilations and strides has default value which is already supported by the code
|
@tjingrant made multiple changes to the code. please review it again. Thank you! |
|
@sdmonov Please resolve the branch conflicts with master. |
|
@chinhuang007 done |
|
This pull request introduces 2 alerts and fixes 1 when merging 487e24e into 39ea046 - view on LGTM.com new alerts:
fixed alerts:
|
|
@sdmonov I merge the PR. But there are two minor places to be cleaned up. Please check the LGTM analysis alerts (basically extra and unused imports) and fix them in the average pool PR. Thanks! |
|
@chinhuang007 sure I saw the LGTM alerts. I think I can merge them with the average pool PR. |
5 participants
Version 10 support includes dilations and ceil_mode.
This is new implementation of max pooling that reduces dilated pooling into regular pooling by calculating and extracting (using tf.gather_nd) the values from the input Tensor that match the kernel, strides and dilations. It utilizes tf.nn.pool or tf.nn.max_pool_with_argmax to extract the final max values/indices. It will utilize build-in tensorflow ops (such as tf.nn.pool, tf.nn.max_pool_with_argmax or tf.nn.dilation2d) whenever possible. strict argument of the CLI is not processed as v10 max_pool will always generate semantically equivalent tensorflow model to the orignal model.
Also added multiple unit tests for max_pooling.