-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[prototype] Speed up autocontrast_image_tensor
#6935
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
Conversation
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.
LGTM if CI is green (with the caveat of #6934). Thanks Vasilis!
The failure happens in the consistency tests which test for equality by default
You can add this vision/test/test_prototype_transforms_consistency.py Lines 167 to 168 in 10d47a6
to vision/test/test_prototype_transforms_consistency.py Lines 247 to 254 in 10d47a6
for reasonable default tolerances. We needed to this for a few ops where we changed algorithms and in turn got slight deviations from v1. |
@pmeier Thanks for the advice. Worked locally. I'll rerun the tests to be sure. |
The failing test is the false positive that @vfdev-5 is currently investigating (see #6933 (comment)) |
Summary: * Performance optimization for autocontrast * Fixing tests Reviewed By: NicolasHug Differential Revision: D41265202 fbshipit-source-id: cd1f9f777ecf56168def256a2ef04335a602684b
Related to #6818
A performance improvement for uint8 images:
fn2 is the submitted variant. It is 30% faster on CPU but 15% slower on GPU.
fn3 is another candidate (not included on this PR). It's 13% faster on CPU and about the same on GPU. Here is the implementation:
In offline discussions with @pmeier and @vfdev-5 we decided to choose fn2 because it optimizes for the most common use-case which is doing the process on CPU. Moreover the fn3 variant uses the
out=
idiom which doesn't play nice with autograd.cc @vfdev-5 @bjuncek @pmeier