Skip to content

Convert unknown rank image to 4D image #330

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 10 commits into from
Jul 7, 2019

Conversation

facaiy
Copy link
Member

@facaiy facaiy commented Jul 4, 2019

Fix #243

  1. The name *_4D_image looks kind of confusing, any good idea?
  2. I didn't check all image ops and see if they can work well with unknown rank image, perhaps we should file a new issue with check list.

@facaiy facaiy added the image label Jul 4, 2019
@facaiy facaiy requested a review from WindQAQ July 4, 2019 13:33
@facaiy facaiy requested review from seanpmorgan and a team July 4, 2019 13:33
Copy link
Contributor

@kyleabeauchamp kyleabeauchamp left a comment

Choose a reason for hiding this comment

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

Overall this looks good and is a nice cleanup of the image code. My only question before merging is whether we need to be more explicit about what assumptions we're making regarding the input shape. E.g., do we need to say in docstrings that we explicitly support (N, H, W, C), (H, W, C), and (H, W) inputs and assume that channel (if present) is last? Maybe we can assume the user knows some of this, but it might be good to be explicit in the docstrings.

Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Thank you, Facai! May related to #320 (docstring)

@facaiy facaiy force-pushed the BUG/image_op_with_tf_dataset branch from 61463a0 to eaf339e Compare July 5, 2019 05:14
@facaiy
Copy link
Member Author

facaiy commented Jul 5, 2019

@kyleabeauchamp Good question, Kyle, I didn't notice that. I think *_4D_image methods work for both channel-last and channel-first format, so I revise the corresponding code comments. And for rotate, transform methods, they are designed only for channel-last format which has been written in the corresponding documents.

@WindQAQ It's wired that I find all tests pass without tf.control_dependencies. For safety, I have add the op as suggested :-)

All comments has been solved, can you take a look again, Kyle and Tzu-Wei?

@WindQAQ
Copy link
Member

WindQAQ commented Jul 5, 2019

@facaiy thank you! @kyleabeauchamp, Kyle, feel free to merge this after taking a look.

@facaiy
Copy link
Member Author

facaiy commented Jul 5, 2019

Forget to mention: *_4D_image and get_ndims are only visible in the image module by now, namely, they are not public endpoint of addons.

Copy link
Contributor

@kyleabeauchamp kyleabeauchamp left a comment

Choose a reason for hiding this comment

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

LGTM

@kyleabeauchamp kyleabeauchamp merged commit 96c3308 into tensorflow:master Jul 7, 2019
@facaiy facaiy deleted the BUG/image_op_with_tf_dataset branch July 8, 2019 05:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make image transforms compatible with TF Dataset
4 participants