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.

Sign up for GitHub

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

Jump to bottom

RandomGaussianBlur crashes when factor=0 #1968

Merged
merged 4 commits into from
Jul 31, 2023
Merged

RandomGaussianBlur crashes when factor=0 #1968

merged 4 commits into from
Jul 31, 2023

Conversation

muxamilian
Copy link
Contributor 

>>> import keras_cv
Using TensorFlow backend
>>> blurrer = keras_cv.layers.RandomGaussianBlur(3, 0.)
>>> blurrer.get_random_transformation()
(<tf.Tensor: shape=(3, 1, 1, 1), dtype=float32, numpy=
array([[[[nan]]],


       [[[nan]]],


       [[[nan]]]], dtype=float32)>, <tf.Tensor: shape=(1, 3, 1, 1), dtype=float32, numpy=
array([[[[nan]],

        [[nan]],

        [[nan]]]], dtype=float32)>)

What does this PR do?

Fixes # (issue)

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?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@muxamilian
RandomGaussianBlur crashes when factor=0
9793756 
```>>> import keras_cv
Using TensorFlow backend
>>> blurrer = keras_cv.layers.RandomGaussianBlur(3, 0.)
>>> blurrer.get_random_transformation()
(<tf.Tensor: shape=(3, 1, 1, 1), dtype=float32, numpy=
array([[[[nan]]],


       [[[nan]]],


       [[[nan]]]], dtype=float32)>, <tf.Tensor: shape=(1, 3, 1, 1), dtype=float32, numpy=
array([[[[nan]],

        [[nan]],

        [[nan]]]], dtype=float32)>)
```
ianstenbit
ianstenbit suggested changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@ianstenbit ianstenbit left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think we can fix the numerical issue pretty simply instead of just documenting it 😄

keras_cv/layers/preprocessing/random_gaussian_blur.py Outdated Show resolved Hide resolved
ianstenbit
ianstenbit suggested changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@ianstenbit ianstenbit left a comment

Choose a reason for hiding this comment

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

Thanks -- this is close! Let's just use keras.backend.epsilon instead (see my comment in-line)

keras_cv/layers/preprocessing/random_gaussian_blur.py Outdated Show resolved Hide resolved
ianstenbit
ianstenbit suggested changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@ianstenbit ianstenbit left a comment

Choose a reason for hiding this comment

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

Thanks! Looks like this now breaks some tests -- take a look at my inline comment

keras_cv/layers/preprocessing/random_gaussian_blur.py Outdated
factor = self.factor()
# `factor` must not become too small otherwise numerical issues occur.
# keras.backend.epsilon() behaves like 0 without causing `nan`s
factor = max(self.factor(), keras.backend.epsilon())
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is breaking tests -- probably because self.factor() returns a tf Tensor and epsilon is a python float.

Perhaps better would be to say factor = self.factor() + keras.backend.epsilon()

You can run the test locally to verify that this isn't breaking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be very convenient if the tests ran on each update of the pull request automatically. I think the issue could be fixed now. Can you rerun the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested it locally, it seems to work now. I think the problem was that tensorflow has problems compiling the max.

Copy link
Contributor

Choose a reason for hiding this comment

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

They do run automatically for folks who have contributed before. However, you should always run tests locally instead of depending on CI to run tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Hopefully CI will be happy here as well.

When using max in TF you need to use the tf-built in version, not the Python version, if you want to use TF tensors and have it compile into the TF graph. So in this case we could have used the TF version, but just adding the constant value also works just fine 😄

Copy link
Contributor Author

@muxamilian muxamilian Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In the meantime I tested it locally and it worked. I couldn't find documentation on how to run the tests in the README.md but it is not so hard to figure out. Could be improved maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI tests look good to me except some unrelated formatting issue in some other code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! The formatting issue is unrelated and I have a fix open in #1993.

We have some (limited) guidance on running tests here

@ianstenbit
Copy link
Contributor

/gcbrun

ianstenbit
ianstenbit approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@ianstenbit ianstenbit left a comment

Choose a reason for hiding this comment

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

Thank you!

@ianstenbit ianstenbit merged commit e84df0c into keras-team:master Jul 31, 2023
8 of 9 checks passed
ghost pushed a commit to y-vectorfield/keras-cv that referenced this pull request Nov 16, 2023
@muxamilian
RandomGaussianBlur crashes when factor=0 (keras-team#1968)
d29146d 
* RandomGaussianBlur crashes when factor=0

```>>> import keras_cv
Using TensorFlow backend
>>> blurrer = keras_cv.layers.RandomGaussianBlur(3, 0.)
>>> blurrer.get_random_transformation()
(<tf.Tensor: shape=(3, 1, 1, 1), dtype=float32, numpy=
array([[[[nan]]],


       [[[nan]]],


       [[[nan]]]], dtype=float32)>, <tf.Tensor: shape=(1, 3, 1, 1), dtype=float32, numpy=
array([[[[nan]],

        [[nan]],

        [[nan]]]], dtype=float32)>)
```

* Make sure `factor` cannot become too small

* Use keras.backend.epsilon() instead of 0.01

* Update random_gaussian_blur.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianstenbit ianstenbit ianstenbit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muxamilian @ianstenbit