Add RandomBrightness preprocessing layer. #122

qlzh727 · 2022-02-09T23:20:31Z

Fix #87.

As a demo (produced by the demo script).

Original image:

Augmented image (scale by -0.5 ~ 0.5

randomly selected)

bhack · 2022-02-09T23:32:30Z

keras_cv/layers/preprocessing/random_brightness.py

+  During inference time, the output will be identical to input. Call the layer with
+  training=True to adjust brightness of the input.
+
+  Note that same brightness adjustment will be apply to all the images in the same


Is this what we want? Does this have any positive/negative significant impact on convergence?
https://github.com/tensorflow/models/blob/master/official/vision/beta/projects/simclr/dataloaders/preprocess_ops.py#L30

great link - yeah, if possible we should avoid applying the same augmentations to an entire batch unless it is extremely difficult to avoid doing so.

Also if it is the same on the entire batch why we are not just layer wrapping:
https://www.tensorflow.org/api_docs/python/tf/image/random_brightness

Also as we have discussed in #91 (comment) we still need to evaluate if we want to expose the op in the public API, keep it only in the keras-cv private API or just embedded in the layer itself.

In the case we want to expose the op in the public API are we going to almost duplicate tf.image.random_brightness?

Updated. The new implementation will generate different adjustment for each of the image in the batch.

On a side note, tf.image.random_brightness has some assumption about the dtype and the range of the value. Eg when dtype is int, it expect the value to range from [0, 255]. When the dtype is float, it will assume the value is ranged [0.0, 1.0]. This means if you give it a float32 image with value [0, 255], then it will almost adjust nothing, since the delta is only [0, 1] and not scaled with * 255.

The current implementation in this PR is only expecting RGB colored image, and all the dtypes should work.

Yes this one is true, but I think that we are entering in another policy here where forking ops is preferred then PR Tensorflow (core).

I think the difference between the current PR and tf ops is quite large and have different assumption

Scale the delta based on the input dtype (this is the major one, and has been apply to all the tf.image ops, which I don't think can be easily changed)

Support of batch inputs. All the existing tf.image ops only take care of single image input. Updating all of them to have batch input support might require more work (but doable).

Output value clip. (Minor and doable).

Uneven delta range. (Minor and doable).

I am not sure who is the active API owner of tf.image ops, @fchollet for more inputs.

Are you sure?

In the end probably it is better to take the ownership of tf.image here and handle the deprecation warning PRs in TF instead of growing incompatible API between TFA/TF/Keras-*.

Then I understand the everyone care only about its ownership but I think that we could improve the library perception to the users/contributors if we think as an ecosystem.

Oh, you are right, my mistake. The current PR apply different adjustment to each image, but the tf.image apply the same adjustment.

Checked with @fchollet offline. It seems that there isn't an active owner for tf.image at this moment. We will have some discussion about how we are going to move forward.

The current PR apply different adjustment to each image, but the tf.image apply the same adjustment.

Yes like your first original implementation here.
In the end as I've explained it is more an ecosystem coordination/harmonization topic then just forking ops with slightly incompatible behaviors so I've mentioned you with a new comment at #74 (comment).
I hope that we could track this general topic there.

keras_cv/layers/preprocessing/random_brightness.py

LukeWood · 2022-02-09T23:43:24Z

keras_cv/layers/preprocessing/random_brightness.py

+  batch.
+
+  Args:
+    scale: Float or a list/tuple of 2 floats between -1.0 and 1.0. The scale is


should we expose image pixel boundaries as well? I'm starting to think we should support all formats for pixel values (i..e [0,1], [-1, 1], [0, 255]).

This seems like a decision we should make and keep consistent across all of our layers

For brightness/contrast and any color related augmentation, I feel we should stay with RGB color (0~255). The normalization of the input (for model) should happen after the color adjustment.

I also think so, like there is a separate rescaling layer that can be used for pixel normalization after all transformation.

LukeWood · 2022-02-09T23:46:34Z

Thanks Scott for the feature add!

…f the image in the batch

qlzh727 · 2022-02-10T06:35:50Z

It seems that the test for random_cutout are failing. We should fix or disable it for now.

LukeWood · 2022-02-10T06:51:10Z

It seems that the test for random_cutout are failing. We should fix or disable it for now.

Weird - I just ran locally and it worked. Let me look into what's going on here.

bhack · 2022-02-10T18:46:47Z

keras_cv/layers/preprocessing/random_brightness.py

+            false_fn=lambda: inputs,
+        )
+
+    def _brightness_adjust(self, image):


Do we want to standardize image or images?

mattdangerw · 2022-02-11T01:18:07Z

A couple comments on consistency within Keras. It seems like bad UX that:

Random contrast will live and be documented in core Keras, as tf.keras.layers.RandomConstrast.
Random brightness live will and be documented here as keras_cv.layers.RandomBrightness.
The two layers will have different expectations around input ranges.

I am not super opinionated on a solution. But I think we should:

Consider landing this in core keras, or aliasing RandomContrast here, so the two can be used in the same way.
If we stick with the 0, 255 range here, consider doing the same for RandomContrast, if no one is relying on the current behavior.

bhack · 2022-02-11T01:33:35Z

@mattdangerw Also internally tf.keras.layers.RandomConstrast implementaton relies on the tf.image ops:

https://github.com/keras-team/keras/blob/v2.8.0/keras/layers/preprocessing/image_preprocessing.py#L1183

Here in the brightness the ops is forked with a slightly different behaviour.

LukeWood · 2022-02-11T01:50:57Z

A couple comments on consistency within Keras. It seems like bad UX that:

Random contrast will live and be documented in core Keras, as tf.keras.layers.RandomConstrast.

Random brightness live will and be documented here as keras_cv.layers.RandomBrightness.

The two layers will have different expectations around input ranges.

I am not super opinionated on a solution. But I think we should:

Consider landing this in core keras, or aliasing RandomContrast here, so the two can be used in the same way.

If we stick with the 0, 255 range here, consider doing the same for RandomContrast, if no one is relying on the current behavior.

I like the idea of aliasing RandomContrast. It will quickly become an outlier as more and more image based preprocessing layers become hosted in KerasCV. Changing the old behavior to expect [0, 255] seems reasonable if KPLs have not been around long enough that this breaking change will be too difficult to get submitted.

mattdangerw · 2022-02-11T02:45:01Z

I like the idea of aliasing RandomContrast. It will quickly become an outlier as more and more image based preprocessing layers become hosted in KerasCV. Changing the old behavior to expect [0, 255] seems reasonable if KPLs have not been around long enough that this breaking change will be too difficult to get submitted.

Unless we do some sort of bulk solution, I think that tension will continue to persist somewhat given that there are 10ish image preprocessing layers in core Keras.

I would actually vote for RandomBrightness in core keras. That would give us an informal split of the "really simple" image augmentation (flips, rotations, brightness, contrast) living in core, and the more complex (cutmix, cutout, solarization, histogram equalization, randaugment) living in keras-cv. I would find that somewhat intuitive, and we can (and probably have to) cross link from one to the other in our documentation.

LukeWood · 2022-02-11T05:56:40Z

I like the idea of aliasing RandomContrast. It will quickly become an outlier as more and more image based preprocessing layers become hosted in KerasCV. Changing the old behavior to expect [0, 255] seems reasonable if KPLs have not been around long enough that this breaking change will be too difficult to get submitted.

Unless we do some sort of bulk solution, I think that tension will continue to persist somewhat given that there are 10ish image preprocessing layers in core Keras.

I would actually vote for RandomBrightness in core keras. That would give us an informal split of the "really simple" image augmentation (flips, rotations, brightness, contrast) living in core, and the more complex (cutmix, cutout, solarization, histogram equalization, randaugment) living in keras-cv. I would find that somewhat intuitive, and we can (and probably have to) cross link from one to the other in our documentation.

yeah, that's a really good point. I underestimated how many image augmentations were in core Keras. RandomBrightness does seem to fit the mould a bit better in core Keras.

Ok, I'm on board to add it there.

bhack · 2022-02-11T10:22:00Z

Honestly we are going to strongly confuse users on the namespace to use or the wheel to install to find a specific CV preprocessing layer.
I suppose that we are going to also confuse contributors on what repo they are going to propose a PR/ticket for CV preprocessing ops.

How solarize is different from brightness? It is hard to semantically grasp this namespace difference as the TF user base has both in the same tf.image namespace:

https://www.tensorflow.org/api_docs/python/tf/image

For brightness adjustment, the value should be same for all the channels. The existing implementation was generating the different value across the channel.

qlzh727 · 2022-02-11T18:38:52Z

I like the idea of aliasing RandomContrast. It will quickly become an outlier as more and more image based preprocessing layers become hosted in KerasCV. Changing the old behavior to expect [0, 255] seems reasonable if KPLs have not been around long enough that this breaking change will be too difficult to get submitted.

Unless we do some sort of bulk solution, I think that tension will continue to persist somewhat given that there are 10ish image preprocessing layers in core Keras.
I would actually vote for RandomBrightness in core keras. That would give us an informal split of the "really simple" image augmentation (flips, rotations, brightness, contrast) living in core, and the more complex (cutmix, cutout, solarization, histogram equalization, randaugment) living in keras-cv. I would find that somewhat intuitive, and we can (and probably have to) cross link from one to the other in our documentation.

yeah, that's a really good point. I underestimated how many image augmentations were in core Keras. RandomBrightness does seem to fit the mould a bit better in core Keras.

Ok, I'm on board to add it there.

I also like the idea of having core image KPL in keras core, and have extended KPL in keras-cv, in the meantime, the core image KPL should probably be aliasing here.

For the behavior difference of [0, 255] vs [0, 1] in the existing keras image KPL, we should probably fix the discrepancy and unify to [0, 255]. This means we will update the image KPL in core keras.

fchollet

Thanks for the PR!

fchollet · 2022-02-14T23:11:30Z

keras_cv/layers/preprocessing/random_brightness.py

+import tensorflow as tf
+
+_SCALE_VALIDATION_ERROR = (
+    "The `scale` should be number or a list of two numbers "


"The scale argument should be a number (or a list of two numbers) "
"in the range [-1.0, 1.0]. "

fchollet · 2022-02-14T23:12:07Z

keras_cv/layers/preprocessing/random_brightness.py

+    batch.
+
+    Args:
+        scale: Float or a list/tuple of 2 floats between -1.0 and 1.0. The scale is


For naming consistency with other preprocessing layers, we should use factor, see e.g. https://keras.io/api/layers/preprocessing_layers/image_augmentation/random_rotation/

fchollet · 2022-02-14T23:13:22Z

keras_cv/layers/preprocessing/random_brightness.py

+            When -1 is chosen, the output image will be black, and when 1.0 is
+            chosen, the image will be fully white. When only one float is provided,
+            eg, 0.2, then -0.2 will be used for lower bound and 0.2 will be used for
+             upper bound.


Should we also support integers? Interpreted as a range in absolute levels, e.g. 32 would be interpreted as a +-32 shift of the channels

fchollet · 2022-02-14T23:14:33Z

keras_cv/layers/preprocessing/random_brightness.py

+        if input_number > 1.0 or input_number < -1.0:
+            raise ValueError(_SCALE_VALIDATION_ERROR + f"Got {input_number}")
+
+    def call(self, inputs, training=None):


Augmentation preprocessing layers should default to training=True (default should be to apply the transformation). You can assume that training is a Python bool in what follows.

This change is ported from keras-team/keras-cv#122. This layer is considered as a core KPL, since we already have RandomContrast. All the image related KPL will be exposed in the keras-cv later as well. PiperOrigin-RevId: 428849704

This change is ported from keras-team/keras-cv#122. This layer is considered as a core KPL, since we already have RandomContrast. All the image related KPL will be exposed in the keras-cv later as well. PiperOrigin-RevId: 429127641

This change is ported from keras-team/keras-cv#122. This layer is considered as a core KPL, since we already have RandomContrast. All the image related KPL will be exposed in the keras-cv later as well. PiperOrigin-RevId: 429127641 Change-Id: Ide2609d3f2c8d1f33260c99b8607f9df0b289200

qlzh727 · 2022-02-21T00:15:00Z

Closing this PR now. Since we have keras.layers.RandomContrast, Keras team decided to keep the RandomBrightness in the core keras as well since both of them are very close to each other. See https://github.com/keras-team/keras/blob/9628af85a0a2cb04cf433b1ad991017b70ae2005/keras/layers/preprocessing/image_preprocessing.py#L1393 for more details.

* Add keras_core.layers.Attention * Address style comments * Fix jax

qlzh727 added 2 commits February 9, 2022 14:59

Initial commit for RandomBrightness layer in keras_cv

a948764

Add demo script for random brightness

25366c8

qlzh727 requested review from fchollet and LukeWood February 9, 2022 23:20

bhack reviewed Feb 9, 2022

View reviewed changes

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

LukeWood reviewed Feb 9, 2022

View reviewed changes

qlzh727 added 4 commits February 9, 2022 21:31

Fix format issue

eb16734

More format fix.

a1ccc21

Change the random behavior to produce different adjustment for each o…

3197628

…f the image in the batch

More format fixes by black

02f08c7

Rename the input parameter to be inputs for consistency

4f52f1b

bhack mentioned this pull request Feb 10, 2022

TF Addons CV components #74

Closed

bhack reviewed Feb 10, 2022

View reviewed changes

qlzh727 mentioned this pull request Feb 10, 2022

add RGBShift preprocessing layers #128

Merged

Consoliate the param name to images

1b02875

Bug fix for adjust value generation

e385c3f

For brightness adjustment, the value should be same for all the channels. The existing implementation was generating the different value across the channel.

fchollet reviewed Feb 14, 2022

View reviewed changes

copybara-service bot mentioned this pull request Feb 15, 2022

Add RandomBrightness layer as one of the image KPL. keras-team/keras#16077

Closed

qlzh727 closed this Feb 21, 2022

bhack mentioned this pull request Feb 22, 2022

[Canary test] GridMask: Vectorized function call on the batch dimension #146

Closed

bhack mentioned this pull request Mar 31, 2022

vectorized_map causes tf.function retracing. #241

Open

innat mentioned this pull request Apr 16, 2022

Introduces a Random resized crop layer #203

Closed

bhack mentioned this pull request May 16, 2022

Fix ChannelShuffle #418

Closed

bhack mentioned this pull request Aug 29, 2022

BUG: RandomContrast slows down training 6-fold #581

Closed

qlzh727 deleted the bright branch September 20, 2022 17:49

freedomtan pushed a commit to freedomtan/keras-cv that referenced this pull request Jul 20, 2023

Add Attention layer (keras-team#122)

cab7465

* Add keras_core.layers.Attention * Address style comments * Fix jax

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RandomBrightness preprocessing layer. #122

Add RandomBrightness preprocessing layer. #122

qlzh727 commented Feb 9, 2022 •

edited

Loading

bhack Feb 9, 2022

LukeWood Feb 9, 2022

bhack Feb 10, 2022

bhack Feb 10, 2022

qlzh727 Feb 10, 2022

bhack Feb 10, 2022 •

edited

Loading

qlzh727 Feb 10, 2022

bhack Feb 10, 2022

qlzh727 Feb 10, 2022

bhack Feb 10, 2022

LukeWood Feb 9, 2022

qlzh727 Feb 10, 2022

innat Feb 10, 2022

LukeWood commented Feb 9, 2022

qlzh727 commented Feb 10, 2022

LukeWood commented Feb 10, 2022

bhack Feb 10, 2022

qlzh727 Feb 10, 2022

mattdangerw commented Feb 11, 2022

bhack commented Feb 11, 2022

LukeWood commented Feb 11, 2022

mattdangerw commented Feb 11, 2022

LukeWood commented Feb 11, 2022 •

edited

Loading

bhack commented Feb 11, 2022 •

edited

Loading

qlzh727 commented Feb 11, 2022

fchollet left a comment

fchollet Feb 14, 2022

fchollet Feb 14, 2022

fchollet Feb 14, 2022

fchollet Feb 14, 2022

qlzh727 commented Feb 21, 2022

Add RandomBrightness preprocessing layer. #122

Add RandomBrightness preprocessing layer. #122

Conversation

qlzh727 commented Feb 9, 2022 • edited Loading

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

bhack Feb 10, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeWood commented Feb 9, 2022

qlzh727 commented Feb 10, 2022

LukeWood commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdangerw commented Feb 11, 2022

bhack commented Feb 11, 2022

LukeWood commented Feb 11, 2022

mattdangerw commented Feb 11, 2022

LukeWood commented Feb 11, 2022 • edited Loading

bhack commented Feb 11, 2022 • edited Loading

qlzh727 commented Feb 11, 2022

fchollet left a 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

qlzh727 commented Feb 21, 2022

qlzh727 commented Feb 9, 2022 •

edited

Loading

bhack Feb 10, 2022 •

edited

Loading

LukeWood commented Feb 11, 2022 •

edited

Loading

bhack commented Feb 11, 2022 •

edited

Loading