Skip to content

Fix weird dash lines on ImageProjectiveTransformV2#42077

Closed
Smankusors wants to merge 1 commit intotensorflow:masterfrom
Smankusors:master
Closed

Fix weird dash lines on ImageProjectiveTransformV2#42077
Smankusors wants to merge 1 commit intotensorflow:masterfrom
Smankusors:master

Conversation

@Smankusors
Copy link

Fixes #41989

So, this is before (look at second row):
Figure_1

and after :
Figure_2

And the script to generate these images is here

Let me know if there's something wrong 😉

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Aug 5, 2020
@gbaned gbaned self-assigned this Aug 6, 2020
@gbaned gbaned added comp:core issues related to core part of tensorflow prtype:bugfix PR to fix a bug labels Aug 6, 2020
@gbaned gbaned requested review from hyeygit and tanzhenyu and removed request for tanzhenyu August 6, 2020 04:40
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 6, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 6, 2020
@Smankusors
Copy link
Author

huh...? It seems that I need to update unit tests at tensorflow/python/keras/layers/preprocessing/image_preprocessing.py?

@tanzhenyu
Copy link
Contributor

huh...? It seems that I need to update unit tests at tensorflow/python/keras/layers/preprocessing/image_preprocessing.py?

Yes

@WindQAQ
Copy link
Member

WindQAQ commented Aug 6, 2020

huh...? It seems that I need to update unit tests at tensorflow/python/keras/layers/preprocessing/image_preprocessing.py?

It's at tensorflow/python/keras/layers/preprocessing/image_preprocessing_test.py.

Hi @Smankusors, if you feel difficult doing tests or don't mind that I take over your job, I can also fix it in #41365. Anyway, feel free to reach out me if there is any problem.

@WindQAQ
Copy link
Member

WindQAQ commented Aug 7, 2020

I haven't validated if this fix is correct, but my implementation in addons is tested against scipy (well, just plugin in the map_coordinate function from scipy to TF), and gets the identical results.

tensorflow/addons#1721
https://github.com/scipy/scipy/blob/v1.5.2/scipy/ndimage/src/ni_interpolation.c#L43-L119

@tanzhenyu
Copy link
Contributor

I haven't validated if this fix is correct, but my implementation in addons is tested against scipy (well, just plugin in the map_coordinate function from scipy to TF), and gets the identical results.

tensorflow/addons#1721
https://github.com/scipy/scipy/blob/v1.5.2/scipy/ndimage/src/ni_interpolation.c#L43-L119

I think this is very worth fixing -- would you mind taking it here?

@Smankusors
Copy link
Author

It's at tensorflow/python/keras/layers/preprocessing/image_preprocessing_test.py.

Hi @Smankusors, if you feel difficult doing tests or don't mind that I take over your job, I can also fix it in #41365. Anyway, feel free to reach out me if there is any problem.

oh yeah damn I pasted a wrong path 😅

hmm I will try do this... Should my transform values be added to the unit tests?

@gbaned gbaned removed the ready to pull PR ready for merge process label Aug 7, 2020
@WindQAQ
Copy link
Member

WindQAQ commented Aug 7, 2020

hmm I will try do this... Should my transform values be added to the unit tests?

Yes! You should fix the original one, and probably add some new test cases that will break in current implementation.

@Smankusors
Copy link
Author

Smankusors commented Aug 8, 2020

hmm unsurprisingly this happens on a first test unit (reflect fill, move the image to down by 1 pixel). I also tried apply_affine_transform from keras-preprocessing. It's.... hmm...

reconfirm

(second column is my patch)

It seems the fix is not as simple as I do 😕

@tanzhenyu
Copy link
Contributor

hmm unsurprisingly this happens on a first test unit (reflect fill, move the image to down by 1 pixel). I also tried apply_affine_transform from keras-preprocessing. It's.... hmm...

reconfirm

(second column is my patch)

It seems the fix is not as simple as I do 😕

What's expected_output here?

@Smankusors
Copy link
Author

What's expected_output here?

expected output is from the existing test unit

expected_output = np.asarray(
[[0., 1., 2.],
[0., 1., 2.],
[3., 4., 5.],
[6., 7., 8],
[9., 10., 11]]).reshape((1, 5, 3, 1)).astype(np.float32)

@WindQAQ
Copy link
Member

WindQAQ commented Aug 8, 2020

I fix it in #41365. It's because the nearest interpolation will round the coordinate. So for example, for "WRAP" fill mode, out_coord = -0.5, and len = 4, in_coord = 3.5, and will be rounded to 4. Since this value is larger than len - 1, it will be filled with fill_value_, resulting in weird dash lines.

Reflect:
reflect
Wrap:
wrap

@Smankusors
Copy link
Author

I fix it in #41365. It's because the nearest interpolation will round the coordinate. So for example, for "WRAP" fill mode, out_coord = -0.5, and len = 4, in_coord = 3.5, and will be rounded to 4. Since this value is larger than len - 1, it will be filled with fill_value_, resulting in weird dash lines.

nice, I suppose I will close this PR because it doesn't work 😅

@Smankusors Smankusors closed this Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes comp:core issues related to core part of tensorflow prtype:bugfix PR to fix a bug size:S CL Change Size: Small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Weird dash lines on ImageProjectiveTransformV2

6 participants