Vectorized random saturation #1392

james77777778 · 2023-02-13T08:55:08Z

What does this PR do?

The vectorized version can pass all tests in old keras_cv/layers/preprocessing/random_saturation_test.py.

Then, as discussed in #1386, I copied the old random saturation layer into keras_cv/layers/preprocessing/random_saturation_test.py and wrote two tests to check the consistency.

This is the result:

Input value range [0, 1]: the vectorized version can almost always pass the test with atol=1e-5, rtol=1e-5. I locally set batch size 1024 to test.
Input value range [0, 255]: the vectorized version can only pass the test by setting higher atol that atol=1e-3, rtol=1e-5.

I think the conversion between RGB and HSV might bring the error because the doc of tf.image.rgb_to_hsv says: https://www.tensorflow.org/api_docs/python/tf/image/rgb_to_hsv

The output is only well defined if the value in images are in [0,1].

I add a benchmark script benchmarks/vectorized_random_saturation.py to show the improvement.

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue? Please add a link
to it if that's the case.
Did you write any new necessary tests?
If this adds a new model, can you run a few training steps on TPU in Colab to ensure that no XLA incompatible OP are used?

Who can review?

@LukeWood

keras_cv/layers/preprocessing/random_saturation_test.py

keras_cv/layers/preprocessing/random_saturation.py

LukeWood

LGTM with a few small changes.

Great PR. Thanks a lot for your hard work.

LukeWood · 2023-02-13T19:39:10Z

@james77777778 thanks for the awesome contribution!

- use ellipsis to prevent dimension error in adjust_factors - rename s_channel_batch to s_channel - fix not implement error for augment_bounding_boxes and augment_labels - remove serialization registration in OldRandomSaturation

james77777778 · 2023-02-14T02:11:37Z

Hi @LukeWood
Thanks for the review

I have updated the codes according to your comments and now can pass all relevant tests:

pytest keras_cv/layers/preprocessing/ragged_image_test.py
pytest keras_cv/layers/preprocessing/random_saturation_test.py
pytest keras_cv/layers/preprocessing/with_labels_test.py
pytest keras_cv/layers/preprocessing/with_segmentation_masks_test.py
pytest keras_cv/layers/preprocessing/with_mixed_precision_test.py

james77777778 · 2023-02-14T03:23:08Z

Once this PR successfully merged, I think I can take a look at RandomBrightness, RandomContrast and RandomHue to complete RandomColorJitter with similar approach?

LukeWood · 2023-02-14T04:24:38Z

/gcbrun

ianstenbit · 2023-02-14T22:11:27Z

keras_cv/layers/preprocessing/random_saturation_test.py

@@ -93,3 +172,29 @@ def test_config(self):
        self.assertTrue(isinstance(config["factor"], core.UniformFactorSampler))
        self.assertEqual(config["factor"].get_config()["lower"], 0.3)
        self.assertEqual(config["factor"].get_config()["upper"], 0.8)
+
+    def test_correctness_with_tf_adjust_saturation_normalized_range(self):


@LukeWood I really like the pattern of A/B testing this before merging these changes, but I'm not sure it belongs in unit tests permanently. wdyt?

Hi @ianstenbit
I'm also wondering if there has better solution. I think the maintenance of old layers will be a problem if

BaseImageAugmentationLayer is modified or deprecated

It is implicit to have a class definition in unit tests

If the consistency of outputs is the major concern, maybe we can extract the computation part (tf.multiply, tf.image, ..., etc) instead of whole class definition for unit tests?

@LukeWood I really like the pattern of A/B testing this before merging these changes, but I'm not sure it belongs in unit tests permanently. wdyt?

So, I see why it might not belong in unit tests permanently - but we do need a way to monitor the deltas here during code reviews. We can't really just ask contributors "did you test this" - we could ask for a colab perhaps, but I think it fits a bit more cleanly in the unit tests.

Personally, I'd opt for include in unit tests for now, then we can remove them all at the end once we feel we have achieved stability.

Okay that sgtm. Thank you Luke and thank you @james77777778 for your work on this!

(One other option we could consider is a/b testing in the benchmark script).

Personally I find that a little bit nicer as then we don't have to have two copies of OldRandomSaturation wdyt?

ok yeah I agree with @ianstenbit - let's put this in the benchmarks.

@LukeWood @ianstenbit
Got it. I'm going to move a/b testing into benchmarks/vectorized_random_saturation.py.

Just one question:
During the migration of vectorizing preprocessing layers, the number of benchmark script might increase rapidly. Should I make new folder to put it?

bhack · 2023-02-15T15:09:02Z

@LukeWood As we are accumulating benchmarks can with these PRs also add a jit_compile version in the benchmark?

If it fails the compilation we can wrap it with try/except (in benchmark) or if in test we could wrap it directly with @unittest.expectedFailure as in:

keras-cv/keras_cv/layers/object_detection/multi_class_non_max_suppression_test.py

Line 50 in 3270143

@unittest.expectedFailure

LukeWood · 2023-02-15T17:34:46Z

@LukeWood As we are accumulating benchmarks can with these PRs also add a jit_compile version in the benchmark?

If it fails the compilation we can wrap it with try/except (in benchmark) or if in test we could wrap it directly with @unittest.expectedFailure as in:

keras-cv/keras_cv/layers/object_detection/multi_class_non_max_suppression_test.py

Line 50 in 3270143

@unittest.expectedFailure

adding an XLA version in a try catch to the benchmarks sounds good to me! Feel free to raise a PR and add it if you'd like this contribution marked

bhack · 2023-02-15T17:49:18Z

adding an XLA version in a try catch to the benchmarks sounds good to me! Feel free to raise a PR and add it if you'd like this contribution marked

Probably it is better to request this in tests coverage of the PR as at least they are run also in CI so we could monitor continuously the compilability over the time

Benchmarks are not run by CI.

ianstenbit

LGTM, but one question about where we should do numerical a/b testing (in comments)

ianstenbit · 2023-02-15T18:15:16Z

keras_cv/layers/preprocessing/random_saturation_test.py

@@ -93,3 +172,29 @@ def test_config(self):
        self.assertTrue(isinstance(config["factor"], core.UniformFactorSampler))
        self.assertEqual(config["factor"].get_config()["lower"], 0.3)
        self.assertEqual(config["factor"].get_config()["upper"], 0.8)
+
+    def test_correctness_with_tf_adjust_saturation_normalized_range(self):


Okay that sgtm. Thank you Luke and thank you @james77777778 for your work on this!

ianstenbit · 2023-02-15T18:16:16Z

keras_cv/layers/preprocessing/random_saturation_test.py

@@ -93,3 +172,29 @@ def test_config(self):
        self.assertTrue(isinstance(config["factor"], core.UniformFactorSampler))
        self.assertEqual(config["factor"].get_config()["lower"], 0.3)
        self.assertEqual(config["factor"].get_config()["upper"], 0.8)
+
+    def test_correctness_with_tf_adjust_saturation_normalized_range(self):


(One other option we could consider is a/b testing in the benchmark script).

Personally I find that a little bit nicer as then we don't have to have two copies of OldRandomSaturation wdyt?

LukeWood · 2023-02-16T04:42:58Z

Thanks you @james77777778 for the great PR!

* Vectorized random saturation * Fix tests - use ellipsis to prevent dimension error in adjust_factors - rename s_channel_batch to s_channel - fix not implement error for augment_bounding_boxes and augment_labels - remove serialization registration in OldRandomSaturation * Fix with_mixed_precision_test * Remove serialization registration in benchmark

Vectorized random saturation

d8aaf9d

LukeWood self-requested a review February 13, 2023 19:34

LukeWood reviewed Feb 13, 2023

View reviewed changes

keras_cv/layers/preprocessing/random_saturation_test.py Outdated Show resolved Hide resolved

LukeWood reviewed Feb 13, 2023

View reviewed changes

keras_cv/layers/preprocessing/random_saturation_test.py Show resolved Hide resolved

LukeWood reviewed Feb 13, 2023

View reviewed changes

keras_cv/layers/preprocessing/random_saturation.py Outdated Show resolved Hide resolved

LukeWood approved these changes Feb 13, 2023

View reviewed changes

james77777778 added 2 commits February 14, 2023 01:39

Fix tests

23963f4

- use ellipsis to prevent dimension error in adjust_factors - rename s_channel_batch to s_channel - fix not implement error for augment_bounding_boxes and augment_labels - remove serialization registration in OldRandomSaturation

Fix with_mixed_precision_test

e003a4a

Remove serialization registration in benchmark

64cbe6e

sebastian-sz mentioned this pull request Feb 14, 2023

Added vectorized Auto Contrast implementation. #1394

Merged

5 tasks

ianstenbit reviewed Feb 14, 2023

View reviewed changes

ianstenbit approved these changes Feb 15, 2023

View reviewed changes

LukeWood merged commit b4513f3 into keras-team:master Feb 16, 2023

james77777778 deleted the preprocess branch February 16, 2023 05:14

This was referenced Feb 16, 2023

Vectorize RandomBrightness, RandomContrast, RandomHue and RandomColorJitter #1405

Closed

Add vectorized RandomBrightness, RandomContrast, RandomHue and RandomColorJitter #1406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vectorized random saturation #1392

Vectorized random saturation #1392

james77777778 commented Feb 13, 2023 •

edited

Loading

LukeWood left a comment

LukeWood commented Feb 13, 2023

james77777778 commented Feb 14, 2023

james77777778 commented Feb 14, 2023

LukeWood commented Feb 14, 2023

ianstenbit Feb 14, 2023

james77777778 Feb 15, 2023

LukeWood Feb 15, 2023

ianstenbit Feb 15, 2023

ianstenbit Feb 15, 2023

LukeWood Feb 15, 2023

james77777778 Feb 16, 2023 •

edited

Loading

bhack commented Feb 15, 2023 •

edited

Loading

LukeWood commented Feb 15, 2023 •

edited

Loading

bhack commented Feb 15, 2023 •

edited

Loading

ianstenbit left a comment

ianstenbit Feb 15, 2023

ianstenbit Feb 15, 2023

LukeWood commented Feb 16, 2023

Vectorized random saturation #1392

Vectorized random saturation #1392

Conversation

james77777778 commented Feb 13, 2023 • edited Loading

What does this PR do?

Before submitting

Who can review?

LukeWood left a comment

Choose a reason for hiding this comment

LukeWood commented Feb 13, 2023

james77777778 commented Feb 14, 2023

james77777778 commented Feb 14, 2023

LukeWood commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james77777778 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

bhack commented Feb 15, 2023 • edited Loading

LukeWood commented Feb 15, 2023 • edited Loading

bhack commented Feb 15, 2023 • edited Loading

ianstenbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeWood commented Feb 16, 2023

james77777778 commented Feb 13, 2023 •

edited

Loading

james77777778 Feb 16, 2023 •

edited

Loading

bhack commented Feb 15, 2023 •

edited

Loading

LukeWood commented Feb 15, 2023 •

edited

Loading

bhack commented Feb 15, 2023 •

edited

Loading