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

[Frontend][Tensorflow] Support explicit_paddings for TF 2.x #7445

Merged
merged 3 commits into from
Feb 20, 2021

Conversation

trevor-m
Copy link
Contributor

@trevor-m trevor-m commented Feb 11, 2021

  • Ignore "_cloned" attr for squeeze.
  • Support explicit padding for conv2d, depthwise conv2d, conv2d transpose and max_pool2d.
  • Support explicit padding for conv3d also, but TF API doesnt allow you to create a conv3d op with explicit padding so there is no unit test.

@FrozenGene
Copy link
Member

FrozenGene commented Feb 18, 2021

I worry about we can not ignore it simply. According to doc: https://www.tensorflow.org/api_docs/python/tf/nn/conv2d

Either the string "SAME" or "VALID" indicating the type of padding algorithm to use, or a list indicating the explicit paddings at the start and end of each dimension. When explicit padding is used and data_format is "NHWC", this should be in the form [[0, 0], [pad_top,pad_bottom], [pad_left, pad_right], [0, 0]]. When explicit padding used and data_format is "NCHW", this should be in the form [[0, 0], [0, 0],[pad_top, pad_bottom], [pad_left, pad_right]].

If explicit_padding is not None, we should apply its values. However, @trevor-m you could make a double check.

@trevor-m
Copy link
Contributor Author

I worry about we can not ignore it simply. According to doc: https://www.tensorflow.org/api_docs/python/tf/nn/conv2d

Either the string "SAME" or "VALID" indicating the type of padding algorithm to use, or a list indicating the explicit paddings at the start and end of each dimension. When explicit padding is used and data_format is "NHWC", this should be in the form [[0, 0], [pad_top,pad_bottom], [pad_left, pad_right], [0, 0]]. When explicit padding used and data_format is "NCHW", this should be in the form [[0, 0], [0, 0],[pad_top, pad_bottom], [pad_left, pad_right]].

If explicit_padding is not None, we should apply its values. However, @trevor-m you could make a double check.

Thanks for the review! That is true, I will update this PR to properly support explicit padding.

@FrozenGene FrozenGene added the status: need update need update based on feedbacks label Feb 19, 2021
@FrozenGene
Copy link
Member

FrozenGene commented Feb 19, 2021

I worry about we can not ignore it simply. According to doc: https://www.tensorflow.org/api_docs/python/tf/nn/conv2d

Either the string "SAME" or "VALID" indicating the type of padding algorithm to use, or a list indicating the explicit paddings at the start and end of each dimension. When explicit padding is used and data_format is "NHWC", this should be in the form [[0, 0], [pad_top,pad_bottom], [pad_left, pad_right], [0, 0]]. When explicit padding used and data_format is "NCHW", this should be in the form [[0, 0], [0, 0],[pad_top, pad_bottom], [pad_left, pad_right]].

If explicit_padding is not None, we should apply its values. However, @trevor-m you could make a double check.

Thanks for the review! That is true, I will update this PR to properly support explicit padding.

Thanks @trevor-m Our convolution op ignore explicit_padding attribute simply is wrong too (Note: be careful of conv2d_transpose: https://www.tensorflow.org/api_docs/python/tf/nn/conv2d_transpose, its explicit padding is not the same as conv2d), not only just pooling op. You could correct it too.Thanks.

@trevor-m trevor-m changed the title [Frontend][Tensorflow] Ignore some TF 2.x attributes [Frontend][Tensorflow] Support explicit_paddings for TF 2.x Feb 19, 2021
@trevor-m
Copy link
Contributor Author

@FrozenGene I've updated the PR with explicit padding support. PTAL

I noticed that while many ops now have the "explicit_paddings" attribute, the ability to actually use explicit padding in the TF python API has only exposed it for a few ops.

@FrozenGene FrozenGene merged commit 5688068 into apache:main Feb 20, 2021
@FrozenGene
Copy link
Member

Thanks @trevor-m Merged.

@FrozenGene FrozenGene added status: accepted and removed status: need update need update based on feedbacks labels Feb 20, 2021
masahi pushed a commit to masahi/tvm that referenced this pull request Feb 22, 2021
)

* Ignore some TF2.0 attributes

* Support explicit padding for conv2d, max_pool, conv3d

* Remove conv3d explicit padding test since TF API doesn't allow it
Lokiiiiii pushed a commit to Lokiiiiii/tvm that referenced this pull request Mar 2, 2021
)

* Ignore some TF2.0 attributes

* Support explicit padding for conv2d, max_pool, conv3d

* Remove conv3d explicit padding test since TF API doesn't allow it
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2021
)

* Ignore some TF2.0 attributes

* Support explicit padding for conv2d, max_pool, conv3d

* Remove conv3d explicit padding test since TF API doesn't allow it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants