-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Add gaussian noise transform #6192 #6233
Add gaussian noise transform #6192 #6233
Conversation
Hi @parth-shastri! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
This PR addresses the issue #6192. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @parth-shastri thanks a lot for the PR.
While I haven't gone through thorough details and verified the implementation fully. I had few code style and comments.
Also,
file is unnecessary. Is it because you are using windows and this file is generated while compiling torchvision from source?
I am sure @vfdev-5 is champion in this domain and will have a deep look 😄
torchvision/transforms/transforms.py
Outdated
Returns: | ||
PIL Image or Tensor: Image added with gaussian noise. | ||
""" | ||
if not isinstance(image, torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm bit hesitant about this implementation. Why not create F.gaussian_noise
that would work well on PIL Images and tensors which we can then use here?
See https://github.com/pytorch/vision/blob/main/torchvision/transforms/functional.py#L1338
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's create a functional op F.gaussian_noise
and at first iteration we can have tensor implementation. If input is PIL, we can convert it to tensor, apply the op and get back PIL image type for the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parth-shastri please create a functional op F.gaussian_noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parth-shastri please address this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes working on that
@parth-shastri Thanks for the contribution. As noted at #6192 (comment), we are currently reviewing the API of Transforms so ideally we don't want to introduce new ones while working on it. Perhaps we could add this directly to the new API as a means of offering incentives to the users to migrate. @vfdev-5 wdyt? |
I think it is OK, this transformation is applied on images and meanwhile we are coding/reviewing new API, we can accept small transforms like this one. |
Co-authored-by: Aditya Oke <[email protected]>
Co-authored-by: Aditya Oke <[email protected]>
@parth-shastri can you please fix failing job "ci/circleci: lint_python_and_config" |
gallery/plot_transforms.py
Outdated
noisy = T.GaussianNoise(mean=0.0, sigma=(0.1, 5)) | ||
noisy_imgs = [noisy(orig_img) for _ in range(4)] | ||
plot(noisy_imgs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives the following result: https://output.circle-artifacts.com/output/job/34f9c3d8-231e-4748-81b9-cda1724a9f16/artifacts/0/docs/auto_examples/plot_transforms.html#gaussiannoise which does not make sense. Let's reduce the number of transformed images and propose acceptable range of sigmas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not keep an accepted range of sigmas, just let the user decide how much distortion does he want ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the documentation images in the documentation.
…th-shastri/vision into add-gaussian-noise-transform
torchvision/transforms/transforms.py
Outdated
Returns: | ||
PIL Image or Tensor: Image added with gaussian noise. | ||
""" | ||
if not isinstance(image, torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parth-shastri please address this comment
addition of type hints Co-authored-by: Aditya Oke <[email protected]>
Co-authored-by: vfdev <[email protected]>
added isinstance instead of F._is_pil_image()
|
You can visualise the results by building the docs. You can just try doing a grid search (or just try a few values and post the images here) cc @vfdev-5 any suggestions for Sigma ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @parth-shastri
I left few other comment to improve the PR
], | ||
) | ||
# add the gaussian noise with the given mean and sigma. | ||
normalize_img = img / 255.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't make this assumption that img
range is [0, 255].
By default, we assume: [0, 1] for float images and [0, 255] for uint8 RGB images.
Let's not rescale image range and let user pick appropriate mean
and sigma
according to their data range.
@@ -748,8 +748,6 @@ def gaussian_blur(img: Tensor, kernel_size: List[int], sigma: List[float]) -> Te | |||
kernel.dtype, | |||
], | |||
) | |||
|
|||
# padding = (left, right, top, bottom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this comment
torchvision/transforms/functional.py
Outdated
if sigma is None: | ||
raise ValueError("The value of sigma cannot be None.") | ||
|
||
if sigma is not None and not isinstance(sigma, (int, float, list, tuple)): | ||
raise TypeError(f"sigma should be either float or sequence of floats. Got {type(sigma)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is unclear here about whether sigma is Optional
or not. Docstring and typehint says optional, but you raise an error if sigma is None. Let's remove optional and assume that sigma is not None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parth-shastri check above comment and the code. Why do you check sigma is None
if sigma is not intended to be Optional
.
Co-authored-by: vfdev <[email protected]>
Hi @vfdev-5 Anything more that you want me to do |
@vfdev-5 is there anything more to do? |
@oke-aditya @vfdev-5 can you please highlight any changes I need to make in order to merge this pull request? |
Hi. I think it would be valuable to have Gaussian Noise as a transformation. What is the state of this at the moment? Should I take over this PR in order to finally merge it? |
The problem with this PR right now is that it should add code compatible to the new v2 API. |
Hi I can take this up, do you want me to create another pull request compatible with the new v2 API or shall I continue here? |
Thanks a lot for your PR @parth-shastri . We just merged #8381 which adds gaussian noise, but thanks again for the original implementation! |
Fixes #6192
Adds GaussianNoise transform functionality to the torchvision.transform.
-Basic GaussianNoise augmentation technique