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

Deprecate ImageProjectiveTransformV2 #1779

Closed
WindQAQ opened this issue May 4, 2020 · 8 comments · Fixed by #1914
Closed

Deprecate ImageProjectiveTransformV2 #1779

WindQAQ opened this issue May 4, 2020 · 8 comments · Fixed by #1914
Labels
good first issue help wanted Needs help as a contribution image

Comments

@WindQAQ
Copy link
Member

WindQAQ commented May 4, 2020

ImageProjectiveTransformV2 seems to be integrated into core TF at tensorflow/tensorflow@2699281 in Jan. We have to deprecate the use of it.

https://github.com/tensorflow/addons/blob/master/tensorflow_addons/custom_ops/image/cc/kernels/image_projective_transform_op.cc

Also, it was unclear to me that ImageProjectiveTransformV2 has been integrated into TF many months ago. Maybe we should check new stuff in TF regularly. Thanks @bhack for pointing out!

cc @tensorflow/sig-addons-maintainers

@seanpmorgan

This comment has been minimized.

@tanzhenyu
Copy link
Contributor

@bhack this issue is for what needs to be done to deprecate ImageProjectTransformV2.

Please create a new issue to discuss how we can clarify the tf.image, tfa.image, and Keras image preprocessing relationship. Alternatively, add it to the meeting agenda. and let us know who you want us to try to get to the meeting for in person discussion.

Yeah I believe those are separate issues. For ImageProjectiveTransformV2 we could either 1) completely remove it from tfa.image, or 2) keep a reference to core.

@AakashKumarNain
Copy link
Member

I believe any redundancy is bad. If we have this in TF core, then I suggest we start with a deprecation warning and remove it gradually in the next version

@gabrieldemarmiesse
Copy link
Member

gabrieldemarmiesse commented May 4, 2020

Thanks @bhack for bringing this to our attention! I believe we should replace the body of our implementation and call the one in TF while keeping the same API on our side for backward compatibility. If the TF implementation is the same as ours, the tests should pass. Though we should also keep the C++/CUDA code on our side for a while because if we remove the custom op, people trying to load their models will get an error.

But the biggest problem right now is that we can't access their implementation because it's not exposed in the public API (as far as I know). We could ask the TF team to expose it for TF 2.3? At least it doesn't seem that we're in a hurry.

@seanpmorgan

This comment has been minimized.

@gabrieldemarmiesse

This comment has been minimized.

@gabrieldemarmiesse
Copy link
Member

Cleaning up the discussion for the future readers.

@bhack
Copy link
Contributor

bhack commented May 4, 2020

I removed all the comments here and also the original comment on the @WindQAQ PR that generated this ISSUE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue help wanted Needs help as a contribution image
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants