Navigation Menu

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

Correct Tensor order for dilation2D #30277

Conversation

gabriel-vanzandycke
Copy link
Contributor

@gabriel-vanzandycke gabriel-vanzandycke commented Jul 1, 2019

gen_nn_ops.dilation2d seems to be in NHWC while the parent function was asking for NCHW.
I corrected the doc and the check.

RELNOTES: tf.nn.dilation2d now correctly requires its data_format argument to be "NHWC".

`gen_nn_ops.dilation2d` seems to be in `NHWC` while the parent function was asking for `NCHW`.
I corrected the doc and the check.
@tensorflow-bot tensorflow-bot bot added the size:XS CL Change Size: Extra Small label Jul 1, 2019
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@gabriel-vanzandycke
Copy link
Contributor Author

I signed it...

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jul 1, 2019
@rthadur rthadur self-assigned this Jul 1, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Jul 1, 2019
@rthadur rthadur requested a review from alextp July 1, 2019 17:02
@rthadur rthadur added the comp:ops OPs related issues label Jul 1, 2019
@alextp alextp requested review from reedwm and removed request for alextp July 1, 2019 17:07
@reedwm reedwm requested review from sjamesr and removed request for reedwm July 1, 2019 18:05
@sjamesr
Copy link
Contributor

sjamesr commented Jul 3, 2019

Thank you for the change, I think it's correct but let me follow up with more knowledgeable folks in the TF team before I approve it.

Copy link
Member

@martinwicke martinwicke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is correct. Bummer.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Jul 3, 2019
@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 3, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 3, 2019
@martinwicke
Copy link
Member

@gabriel-vanzandycke can you also correct the converter tool to add the data_format arg? It should be parallel to the entry for erosion2d: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tools/compatibility/tf_upgrade_v2.py#L1525

@tomerk The converter didn't do the right thing here, can you review the change to make sure it will once @gabriel-vanzandycke is done?

@tensorflow/api-owners This is a bug affecting the API: the accepted string has changed. The old state is a bug, we need to fix it. I added relnotes.

Copy link
Member

@martinwicke martinwicke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the converter code, see other comment.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jul 3, 2019
Copy link
Member

@martinwicke martinwicke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in, I'll submit the converter change separately.

@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label Aug 1, 2019
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Aug 1, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 1, 2019
@rthadur rthadur removed the ready to pull PR ready for merge process label Aug 1, 2019
@rthadur rthadur added the ready to pull PR ready for merge process label Aug 1, 2019
tensorflow-copybara pushed a commit that referenced this pull request Aug 1, 2019
This is the missing part of #30277.

This will make the converter script produce invalid code (until #30277 is in), but it's no worse than today at least.

PiperOrigin-RevId: 261195994
@tensorflow-copybara tensorflow-copybara merged commit 2e4d395 into tensorflow:master Aug 1, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Aug 1, 2019
tensorflow-copybara pushed a commit that referenced this pull request Aug 1, 2019
…e-patch-dilation2d

PiperOrigin-RevId: 261198637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:ops OPs related issues ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants