-
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
Added antialias option to transforms.functional.resize #3761
Conversation
9257c2e
to
0e10604
Compare
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 this looks great, thanks @vfdev-5 !
I've made a few comments, but none of which is merge-blocking.
I've also merged a PR adding Android CI, let's see if it passes once we rebase this diff, we might need to update the android CMakeLists as well
index_t ids_min = *(index_t*)&data[0][i * strides[0]]; | ||
index_t ids_size = *(index_t*)&data[1][i * strides[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.
Looks like the only difference between this function and the zero_strides
one is this part (plus the line below).
Do you remember how much of a speed-up we got by doing this?
- used pytest parametrize instead unittest - cast to scalar_t ptr - removed interpolate aa files for ios/android keeping original cmake version
@pytest.mark.parametrize('device', ["cpu", ]) | ||
@pytest.mark.parametrize('dt', [None, torch.float32, torch.float64, torch.float16]) | ||
@pytest.mark.parametrize('size', [[96, 72], [96, 420], [420, 72]]) | ||
@pytest.mark.parametrize('interpolation', [BILINEAR, ]) |
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 port! Just 2 nits: we probably don't need the device
and interpolation
parametrizations since they only have one element, so I'd suggest to remove them. Should we need them in the future, it will be very easy to add back
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.
True, I agree but as mentioned in the description, in the follow-up PRs we aim to add more interpolation modes and CUDA support, that's why I kept them.
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!
Summary: * WIP Added antialias option to transforms.functional.resize * Updates according to the review * Excluded these C++ files for iOS build * Added support for mixed downsampling/upsampling * Fixed heap overflow caused by explicit loop unrolling * Applied PR review suggestions - used pytest parametrize instead unittest - cast to scalar_t ptr - removed interpolate aa files for ios/android keeping original cmake version Reviewed By: datumbox Differential Revision: D28473319 fbshipit-source-id: 90b6b04903aeaf0ad198aae63216e6dff59cec75
Related to #2950
Description:
Interpolation with antialias code temporary added to torchvision before moving to pytorch.
C++ code is based on https://github.com/vfdev-5/interpolate-antialiasing#step-22
For a follow-up PRs: