Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[proto] Speed up adjust color ops #6784
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
[proto] Speed up adjust color ops #6784
Changes from all commits
94e918c
fc4f237
58eec29
ffc5c4f
b7b5178
4f3491a
2a5e4d8
a170513
0b55072
a99d6ad
b7fdd39
4117957
a82cf8c
247ed7d
f19edc9
eff6c6f
2397725
f364efc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 like this! Supersedes the work at #6765
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.
Issue:
This clamp_ implementation is enforcing the histogram to take values between 0 and 1 in my case is not working the way I will expect since my images are normalized to values around 0 (ex: -0.2 to 0.8) so the multiplication with the factor of 1 will not return an identical image.
Suggestion:
output = image.mul(brightness_factor).clamp_(image.min(), image.max())
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.
@MiChatz thanks for the feedback. There is (yet unwritten) assumption for color transformations on float images that image range is between [0, 1].
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 saves one conversion in
_rgb_to_gray
in case the input is uint8:_rgb_to_gray
would convert the output of its floating point computation back to uint8 just for the result being converted back to floating point in before thetorch.mean
call.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.
@pmeier actually, doing so the output of
mean
is not the same for uint8 image.As we originally applied uint8 cast in the end of
_rgb_to_gray
we get rid of all floating point values. This is not the case if image is casted to float before_rgb_to_gray
.Here is an example of difference for
mean
:So, finally consistency tests report for example:
and this is a real failure, IMO.
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 that the behavior changes, but IMO repeatedly converting to
uint8
in the computation and thus eliminating intermediate values sounds more like a missed opportunity in the original kernel than a bug now. Thus, I would consider this more like a "bug fix" rather than a BC breaking change. On the other hand, that is not a strong opinion. Not going to block over this.