Use ImageProjectiveTransformV3 for TF >= 2.4.0 #2293

WindQAQ · 2020-12-15T20:46:18Z

Description

Support fill_value and nearest fill_mode with ImageProjectiveTransformV3.

Type of change

Checklist:

I've properly formatted my code according to the guidelines
- By running Black + Flake8
- By running pre-commit hooks
This PR addresses an already submitted issue for TensorFlow Addons
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
This PR contains modifications to C++ custom-ops

How Has This Been Tested?

Test new features.

bot-of-gabrieldemarmiesse · 2020-12-15T20:46:55Z

@sayoojbk @mels630

You are owners of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

WindQAQ · 2020-12-15T20:52:07Z

tensorflow_addons/image/transform_ops.py

@@ -37,6 +37,7 @@ def transform(
    transforms: TensorLike,
    interpolation: str = "NEAREST",
    fill_mode: str = "CONSTANT",
+    fill_value: TensorLike = 0.0,


I have no idea where should we put the new argument. Should we maintain backward compatibility so we have to put it in the end of args? @seanpmorgan WDYT?

Yes this looks like the best position

WindQAQ · 2020-12-15T21:08:58Z

@seanpmorgan Hi Sean, this is only for TF2.4. Our nightly version will pin to both TF2.3 and TF2.4, right?

seanpmorgan · 2020-12-15T21:24:36Z

@seanpmorgan Hi Sean, this is only for TF2.4. Our nightly version will pin to both TF2.3 and TF2.4, right?

Trying to expand our python compatibility to be 2.3+. The wheel will be built against TF2.4 so custom-ops will need TF2.4, but everything else would be great to keep compatibility.

Raising a NotImplementedError for <=2.4 is an option

bhack · 2020-12-15T21:24:47Z

Tracking many TF core issues and PRs my opinion Is that currently TF has a quite strict policy to not backport not only features but also bug fixes from nightly/master. Patch releases are sparse and for security fixes only.

So I think that we need to start to support and promote only the last official TF release.

WindQAQ · 2020-12-15T21:50:41Z

Trying to expand our python compatibility to be 2.3+. The wheel will be built against TF2.4 so custom-ops will need TF2.4, but everything else would be great to keep compatibility.

Raising a NotImplementedError for <=2.4 is an option

I can do a warning for TF<2.4 that fill_value is not supported and is always 0.

Tracking many TF core issues and PRs my opinion Is that currently TF has a quite strict policy to not backport not only features but also bug fixes from nightly/master. Patch releases are sparse and for security fixes only.

So I think that we need to start to support and promote only the last official TF release.

I am more with this proposal though, there is always a loop from "I want the new feature" -> "Please use latest TF" -> "codes does not work in latest TF" -> "Fallback to older TF" -> "I want the new feature" XD

bhack · 2020-12-15T22:22:23Z

I am more with this proposal though, there is always a loop from "I want the new feature" -> "Please use latest TF" -> "codes does not work in latest TF" -> "Fallback to older TF" -> "I want the new feature" XD

Often it Is more like:
There Is a bug -> It Is fixed in the last version, or in nightly/master -> please use the last official or nightly wheels.

bhack · 2020-12-15T23:12:05Z

tensorflow_addons/image/transform_ops.py

+            )
+        else:
+            fill_mode = fill_mode.upper()
+            # TODO(WindQAQ): Get rid of the check once we drop TensorFlow < 2.4 support.


Can we automatically/programmatically fail with https://github.com/tensorflow/addons/blob/master/setup.py#L79?
What do you think? I suppose that this kind of things will stay forever in the code 😉

Eh, we can clean up the codebase as our compatibility matrix moves. See #2294

Yes but It could be easier if we are failing in different points in the sourfe when we move the minimal version or the single version we support. Do you understand what I meant?

aaronmondal

Thanks I need this so much! Was going crazy over the inconsistency between the different fill options for geometric transformations :D.

The only thing I am not so sure about is whether the nearest fill option actually does work.

There should probably be tests for the "nearest" and "bilinear" interpolation modes of transform as well. Currently, the interpolation argument is untested.

The test_bilinear and test_uint8 tests in transform_ops_test.py apply to rotate only, which does make use of transform internally, but I don't think this relation should be assumed by the test case.

tensorflow_addons/image/transform_ops.py

tensorflow_addons/image/translate_ops.py

fsx950223 · 2020-12-16T08:57:09Z

The function should be transfered to official repo? There is a similar function https://github.com/tensorflow/tensorflow/blob/2a7d45c6800a99e4158526e25ec128ddd90e37f7/tensorflow/python/keras/layers/preprocessing/image_preprocessing.py#L625.

WindQAQ · 2020-12-16T09:02:01Z

The function should be transfered to official repo? There is a similar function https://github.com/tensorflow/tensorflow/blob/2a7d45c6800a99e4158526e25ec128ddd90e37f7/tensorflow/python/keras/layers/preprocessing/image_preprocessing.py#L625.

They do not make it public. In fact, they migrate it to KPL without informing us early this year 😭 So we are just doing a fork of private API now.

See tensorflow/addons#2293

bhack · 2020-12-16T12:09:13Z

They do not make it public. In fact, they migrate it to KPL without informing us early this year sob So we are just doing a fork of private API now.

Lets see 😉 ... tensorflow/tensorflow#45742

seanpmorgan · 2020-12-16T15:56:13Z

They do not make it public. In fact, they migrate it to KPL without informing us early this year sob So we are just doing a fork of private API now.

Lets see 😉 ... tensorflow/tensorflow#45742

Agree with the PR, though I don't want to have to wait for a response prior to 0.12 release.

bhack · 2020-12-16T19:12:40Z

Agree with the PR, though I don't want to have to wait for a response prior to 0.12 release.

It is impossible to achieve this for 0.12 cause this will got in TF 2.5 but you can bypass the policy and depend on a private API for this case, as you want.

seanpmorgan · 2020-12-16T19:40:20Z

Agree with the PR, though I don't want to have to wait for a response prior to 0.12 release.

It is impossible to achieve this for 0.12 cause this will got in TF 2.5 but you can bypass the policy and depend on a private API for this case, as you want.

Understand the TF core PR wouldn't be until TF2.5, but what do you mean depend on a private API and bypass the policy? There is no private API in this PR right?

bhack · 2020-12-16T19:48:08Z

Agree with the PR, though I don't want to have to wait for a response prior to 0.12 release.

It is impossible to achieve this for 0.12 cause this will got in TF 2.5 but you can bypass the policy and depend on a private API for this case, as you want.

Understand the TF core PR wouldn't be until TF2.5, but what do you mean depend on a private API and bypass the policy? There is no private API in this PR right?

I meant as an alternative to maintain a (function) fork.

WindQAQ · 2020-12-16T20:14:24Z

tensorflow_addons/image/transform_ops.py

-    interpolation: str = "NEAREST",
-    fill_mode: str = "CONSTANT",
+    interpolation: str = "nearest",
+    fill_mode: str = "constant",


This will not break the compatibility as we always uppercase it when passing into raw ops. Just to canonicalize the documentation and arguments.

aaronmondal · 2020-12-17T00:38:01Z

@WindQAQ Thanks for changing to lowercase. So much prettier :). Would you like me to file a PR for the missing testcases in interpolation?

WindQAQ · 2020-12-17T00:42:12Z

@WindQAQ Thanks for changing to lowercase. So much prettier :). Would you like me to file a PR for the missing testcases in interpolation?

It's more than welcome to do that!

seanpmorgan

LGTM Thanks! Thanks @aaronmondal for the review!

bhack · 2020-12-17T22:37:47Z

Agree with the PR, though I don't want to have to wait for a response prior to 0.12 release.

It is impossible to achieve this for 0.12 cause this will got in TF 2.5 but you can bypass the policy and depend on a private API for this case, as you want.

Understand the TF core PR wouldn't be until TF2.5, but what do you mean depend on a private API and bypass the policy? There is no private API in this PR right?

I meant as an alternative to maintain a (function) fork.

It seems that we need to maintain the fork but if the new policy Is to maintain the public API surface minimal isn't better to handle private API occasional breakage instead of maintain fork in multiple points in the source code?

* Use ImageProjectiveTransformV3 * Mark unsupported things for TF<2.4.0

WindQAQ added 2 commits December 15, 2020 12:41

Use ImageProjectiveTransformV3

610b32a

Test non-zero fill_value

ff44184

boring-cyborg bot added the image label Dec 15, 2020

google-cla bot added the cla: yes label Dec 15, 2020

WindQAQ requested review from seanpmorgan and bhack December 15, 2020 20:46

WindQAQ commented Dec 15, 2020

View reviewed changes

WindQAQ changed the title ~~Image/use image projective transform v3~~ Use image projective transform v3 Dec 15, 2020

WindQAQ added 3 commits December 15, 2020 14:54

Mark unsupported things for TF<2.4.0

49f8cc0

Trivial spelling

5d9a1c2

Trivial spelling

34aa1b2

WindQAQ changed the title ~~Use image projective transform v3~~ Use ImageProjectiveTransformV3 for TF >= 2.4.0 Dec 15, 2020

bhack reviewed Dec 15, 2020

View reviewed changes

aaronmondal suggested changes Dec 16, 2020

View reviewed changes

tensorflow_addons/image/transform_ops.py Outdated Show resolved Hide resolved

tensorflow_addons/image/transform_ops.py Show resolved Hide resolved

tensorflow_addons/image/translate_ops.py Outdated Show resolved Hide resolved

tensorflow_addons/image/translate_ops.py Show resolved Hide resolved

bhack mentioned this pull request Dec 16, 2020

Expose transform tensorflow/tensorflow#45742

Closed

bhack added a commit to bhack/tensorflow that referenced this pull request Dec 16, 2020

Expose transform

8120c5d

See tensorflow/addons#2293

Move arg to the end of args

0361c9c

Change to lowercase

7deae03

WindQAQ commented Dec 16, 2020

View reviewed changes

aaronmondal approved these changes Dec 17, 2020

View reviewed changes

seanpmorgan approved these changes Dec 17, 2020

View reviewed changes

seanpmorgan merged commit 3ca3010 into tensorflow:master Dec 17, 2020

jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020

Use ImageProjectiveTransformV3 for TF >= 2.4.0 (tensorflow#2293)

d9fd5f1

* Use ImageProjectiveTransformV3 * Mark unsupported things for TF<2.4.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ImageProjectiveTransformV3 for TF >= 2.4.0 #2293

Use ImageProjectiveTransformV3 for TF >= 2.4.0 #2293

WindQAQ commented Dec 15, 2020

bot-of-gabrieldemarmiesse commented Dec 15, 2020

WindQAQ Dec 15, 2020 •

edited

Loading

seanpmorgan Dec 16, 2020

WindQAQ Dec 16, 2020

WindQAQ commented Dec 15, 2020 •

edited

Loading

seanpmorgan commented Dec 15, 2020

bhack commented Dec 15, 2020

WindQAQ commented Dec 15, 2020

bhack commented Dec 15, 2020

bhack Dec 15, 2020

seanpmorgan Dec 16, 2020

bhack Dec 17, 2020

aaronmondal left a comment

fsx950223 commented Dec 16, 2020

WindQAQ commented Dec 16, 2020 •

edited

Loading

bhack commented Dec 16, 2020

seanpmorgan commented Dec 16, 2020

bhack commented Dec 16, 2020

seanpmorgan commented Dec 16, 2020

bhack commented Dec 16, 2020

WindQAQ Dec 16, 2020 •

edited

Loading

aaronmondal commented Dec 17, 2020

WindQAQ commented Dec 17, 2020

seanpmorgan left a comment

bhack commented Dec 17, 2020

Use ImageProjectiveTransformV3 for TF >= 2.4.0 #2293

Use ImageProjectiveTransformV3 for TF >= 2.4.0 #2293

Conversation

WindQAQ commented Dec 15, 2020

Description

Type of change

Checklist:

How Has This Been Tested?

bot-of-gabrieldemarmiesse commented Dec 15, 2020

WindQAQ Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

seanpmorgan Dec 16, 2020

Choose a reason for hiding this comment

WindQAQ Dec 16, 2020

Choose a reason for hiding this comment

WindQAQ commented Dec 15, 2020 • edited Loading

seanpmorgan commented Dec 15, 2020

bhack commented Dec 15, 2020

WindQAQ commented Dec 15, 2020

bhack commented Dec 15, 2020

bhack Dec 15, 2020

Choose a reason for hiding this comment

seanpmorgan Dec 16, 2020

Choose a reason for hiding this comment

bhack Dec 17, 2020

Choose a reason for hiding this comment

aaronmondal left a comment

Choose a reason for hiding this comment

fsx950223 commented Dec 16, 2020

WindQAQ commented Dec 16, 2020 • edited Loading

bhack commented Dec 16, 2020

seanpmorgan commented Dec 16, 2020

bhack commented Dec 16, 2020

seanpmorgan commented Dec 16, 2020

bhack commented Dec 16, 2020

WindQAQ Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

aaronmondal commented Dec 17, 2020

WindQAQ commented Dec 17, 2020

seanpmorgan left a comment

Choose a reason for hiding this comment

bhack commented Dec 17, 2020

WindQAQ Dec 15, 2020 •

edited

Loading

WindQAQ commented Dec 15, 2020 •

edited

Loading

WindQAQ commented Dec 16, 2020 •

edited

Loading

WindQAQ Dec 16, 2020 •

edited

Loading