-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Fix glass_blur for float dtype #826
Fix glass_blur for float dtype #826
Conversation
* Fix dtype conversion code in glass_blur * Add a test case for above fix
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.
Thank you very much! These are important fixes.
But could you a little bit rewrite your PR?
@@ -1514,7 +1514,7 @@ def fancy_pca(img, alpha=0.1): | |||
@clipped | |||
def glass_blur(img, sigma, max_delta, iterations, dxy, mode): | |||
coef = MAX_VALUES_BY_DTYPE[img.dtype] | |||
x = np.uint8(cv2.GaussianBlur(np.array(img) / coef, sigmaX=sigma, ksize=(0, 0)) * coef) | |||
x = np.uint8(cv2.GaussianBlur(np.array(img) / coef, sigmaX=sigma, ksize=(0, 0)) * 255) |
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.
Better to simple remove conversion into uint8
and convert to image data type
x = cv2.GaussianBlur(np.array(img) / coef, sigmaX=sigma, ksize=(0, 0)) * coef
x = x.astype(img.dtype)
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.
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.
@Dipet, I agree with removing this normalization. I assume that it was used in the original implementation of glass_blur (https://github.com/hendrycks/robustness/blob/master/ImageNet-C/create_c/make_imagenet_c.py#L300) because skimage works mostly with float images that have values in the range [0.0, 1.0].
@Dipet Thank you for your quick reply.
Of course! Now, I updated my PR.
I modified the test case to allow for the difference between uint8 and float at most 2. If any fix is needed about this issue, please let me know. |
def glass_blur(img, sigma, max_delta, iterations, dxy, mode): | ||
coef = MAX_VALUES_BY_DTYPE[img.dtype] | ||
x = np.uint8(cv2.GaussianBlur(np.array(img) / coef, sigmaX=sigma, ksize=(0, 0)) * coef) | ||
x = cv2.GaussianBlur(np.array(img) / coef, sigmaX=sigma, ksize=(0, 0)) * coef |
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.
Can you remove normalization by coef
?
With these changes we also don't need normalize x
in the and and clip it
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.
@Dipet OK, I see your point.
Can I ask for some advice about the test case?
In the first PR version, I added a test case to check the transformed values of uint8, float32 images are approximately the same.
My intention against the test case is to confirm some dtype operations are done appropriately.
Thanks to your review, now the code has no explicit dtype operations, so I think the test case becomes less effective.
Is it better to remove my test case?
If there is any idea about this issue’s test case, let me know?
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.
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 it is good that we have this test.
In my opinion all transforms in the library must have similar tests to check that transforms work the same for uint8 and float datatypes.
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.
OK, I see. Thanks for the comment.
I found a bug about the
GlassBlur
(not GausssianBlur!) transform.In this PR, I tried to fix the problem.
I would appreciate it if this PR is reviewed.
Problem
The
GlassBlur
transform returns wrong values when an input image's dtype is notuint8
. This is caused by misimplementation of dtype conversion in theglass_blur
function.The following code is a reproducible example.
Note, the result of float32 case is expected to be non-zero.
Fix
I change the following two lines.
cv2.GaussianBlur
should be multiplied by 255 (max uint8) instead ofcoef
(max of input image dtype).x
values whose dtype isuint8
should be divided by 255 instead ofcoef
before passed to thecv2.GaussianBlur
.After the fix, I got the following results.