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

Gaussian Blur layer implementation #143

Closed
wants to merge 1 commit into from

Conversation

artu1999
Copy link
Contributor

implemented Gaussian Blur preprocessing layer using the gaussian blur implemented in tensorflow additions (Reference)

Linked Issue

layer = GaussianBlur(kernel_size=(10,7), sigma=1)

Figure_1

layer = GaussianBlur(kernel_size=(10,7), sigma=2)

Figure_2

@artu1999
Copy link
Contributor Author

artu1999 commented Feb 20, 2022

I have just realised the email I have for author wasn't matching the email on the CLA. In case no changes are required, let me know if I should revert and resubmit.

@bhack
Copy link
Contributor

bhack commented Feb 20, 2022

@qlzh727 Do you know if we have an update from the TF team on tensorflow/tensorflow#39050?

@LukeWood
Copy link
Contributor

FYI @artu1999 we'll need you to sign the CLA to contribute code. Thanks

training = backend.learning_phase()

def _blur(image, kernel_size, sigma):
image = tfa.image.gaussian_filter2d(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't rely on TFA in this repo. We will need to ship our own implementation. Gaussian blurs can actually be implemented through convolution (if tf.image doesn't have anything we can use). Perhaps check the SimCLR repo for a reference implementation

Returns:
images: Blurred input images, same as input.
"""
if training is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assume training is a python bool.

Returns:
images: Blurred input images, same as input.
"""
if training is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assume training is a python bool.

@LukeWood
Copy link
Contributor

Thanks for the PR . I've left some early comments.

@artu1999
Copy link
Contributor Author

@LukeWood I have realised I used the wrong author name, I will make sure to use the correct one for the next commit. In terms of the next steps:

  • Is there any desire of making the Blur random? the implementation in simCLR already does that (Reference). Examples where Random Blur is used:

https://arxiv.org/pdf/2009.00104.pdf

https://arxiv.org/pdf/2006.10955.pdf

I think it might be something very useful as other libraries don't seem to support this.

  • Will start readapting the code so it won't rely on tfa.
  • Include training in docstring

@LukeWood
Copy link
Contributor

@LukeWood I have realised I used the wrong author name, I will make sure to use the correct one for the next commit. In terms of the next steps:

  • Is there any desire of making the Blur random? the implementation in simCLR already does that (Reference). Examples where Random Blur is used:

https://arxiv.org/pdf/2009.00104.pdf

https://arxiv.org/pdf/2006.10955.pdf

I think it might be something very useful as other libraries don't seem to support this.

  • Will start readapting the code so it won't rely on tfa.
  • Include training in docstring

I'd be in favor of support for random behavior. For example, we could allow filter_size to support ranges as well as sigma to support ranges, then randomly sample within the range.

@bhack
Copy link
Contributor

bhack commented Feb 21, 2022

I'd be in favor of support for random behavior. For example, we could allow filter_size to support ranges as well as sigma to support ranges, then randomly sample within the range.

@LukeWood Are we going to always be random inside the batch? Cause I think that this sometimes is going already to complicate the vectorizzation a bit. Do we have any reference about the convergence improvement with in-batch randomization?
If we could randomize only between the batches instead I think that a Random could be more a policy class then something that we need to be supported with every args at preprocessing layer level.

@LukeWood
Copy link
Contributor

I'd be in favor of support for random behavior. For example, we could allow filter_size to support ranges as well as sigma to support ranges, then randomly sample within the range.

@LukeWood Are we going to always be random inside the batch? Cause I think that this sometimes is going already to complicate the vectorizzation a bit. Do we have any reference about the convergence improvement with in-batch randomization? If we could randomize only between the batches instead I think that a Random could be more a policy class then something that we need to be supported with every args at preprocessing layer level.

We are leaning towards guiding users towards implementing:
augment_image

Then relying on XLA to perform optimization. This may be slightly less performant but the entire JAX ecosystem relies on this, so it should actually be fine. We want to minimize dependence between samples so this is optimal.

@bhack
Copy link
Contributor

bhack commented Feb 21, 2022

Yes but also if you want to use XLA or vectorized_map does it make sense to have the preprocessing layer itself Random? Isn't better to have a Random policy class that handle the randomization of the args for the underline preprocessing layer?
In that case this could enable more complex policies still controlling the preprocessing args.

@artu1999
Copy link
Contributor Author

artu1999 commented Feb 22, 2022

Having a higher level Random Blur layer might also be useful in case we want to give users the possibility of applying random MotionBlur and MedianBlur as well. Possibly, we could use an argument to specify which type of random blur the user wants to apply or even apply a random type of Blur for each picture in the batch.

bhack added a commit to bhack/keras-cv that referenced this pull request Feb 22, 2022
As we are discussed in keras-team#143 (comment) this is going to fail (check the CI) cause:
```
ValueError: Input "maxval" of op 'RandomUniformInt' expected to be loop invariant.
```

As I've mentioned in the thread we really need to understand if we want to have randomness inside the batch or between the batches and what kind of impact we have between the computing overhead, contributing speed/code readability  and  network convergence.

Also I don't know if @joker-eph  or @qlzh727 could expose use a little bit the pro and cons of `jit_comile` a function vs using the `vectorized_map` or if they are orthogonal. With many CV transformations  we cannot compile the function as the underline `tf.raw_ops.ImageProjectiveTransformV3 ` op isn't supported by XLA.
@qlzh727
Copy link
Member

qlzh727 commented Feb 22, 2022

@qlzh727 Do you know if we have an update from the TF team on tensorflow/tensorflow#39050?

I don't think there is any active development on that issue at the moment.

@LukeWood
Copy link
Contributor

Yes but also if you want to use XLA or vectorized_map does it make sense to have the preprocessing layer itself Random? Isn't better to have a Random policy class that handle the randomization of the args for the underline preprocessing layer? In that case this could enable more complex policies still controlling the preprocessing args.

This sounds really heavy handed. I think we just want the layers to handle randomization themselves

@bhack
Copy link
Contributor

bhack commented Mar 11, 2022

This sounds really heavy handed. I think we just want the layers to handle randomization themselves

I still believe that I will be harder to remove the implicit randomization over the (hype)parameters space especially as we don't expose the non randomized ops in the internal API.

@LukeWood
Copy link
Contributor

@artu1999 any update here?

@artu1999
Copy link
Contributor Author

@artu1999 any update here?

Sorry for the inactivity, got quite busy with my final university project. I am going to make a commit either tomorrow or the day after with the new code, I just need to fix a couple of things here and there.

@artu1999
Copy link
Contributor Author

Also, quick question: would you be happy with using keras.engine.base_layer.BaseRandomLayer as the class to inherit from or would you prefer to stick with keras.layers.Layer?

@LukeWood
Copy link
Contributor

Hey @artu1999 - Please use BaseImageAugmentationLayer. Please see RandomShear as an example. We will be moving all of our KPL layers to use this:

https://github.com/keras-team/keras-cv/blob/master/keras_cv/layers/preprocessing/random_shear.py#L21

Basically, you write a function augment_image to augment a single image and it handles vectorization and input format processing for you :)

@artu1999
Copy link
Contributor Author

Hey @artu1999 - Please use BaseImageAugmentationLayer. Please see RandomShear as an example. We will be moving all of our KPL layers to use this:

https://github.com/keras-team/keras-cv/blob/master/keras_cv/layers/preprocessing/random_shear.py#L21

Basically, you write a function augment_image to augment a single image and it handles vectorization and input format processing for you :)

Will use that layer then, thanks!

@artu1999 artu1999 mentioned this pull request Apr 1, 2022
@LukeWood
Copy link
Contributor

LukeWood commented Apr 1, 2022

Closing for your other PR. Thanks @artu1999 for starting this process

@LukeWood LukeWood closed this Apr 1, 2022
freedomtan pushed a commit to freedomtan/keras-cv that referenced this pull request Jul 20, 2023
* Fix discrepancy with softmax when axis=None

numpy and jax handled this as an all reduce option
tf as a reduction on the last dimension

* Fix format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants