[prototype] Restore BC on perspective #6902
Conversation
| @@ -1184,38 +1184,38 @@ def reference_inputs_pad_bounding_box(): | |||
| def sample_inputs_perspective_image_tensor(): | |||
| for image_loader in make_image_loaders(sizes=["random"]): | |||
| for fill in [None, 128.0, 128, [12.0], [12.0 + c for c in range(image_loader.num_channels)]]: | |||
| yield ArgsKwargs(image_loader, fill=fill, perspective_coeffs=_PERSPECTIVE_COEFFS[0]) | |||
| yield ArgsKwargs(image_loader, None, None, fill=fill, coefficients=_PERSPECTIVE_COEFFS[0]) | |||
There was a problem hiding this comment.
Nit: While correct, can we pass keywords here to make this call more readable?
| yield ArgsKwargs(image_loader, None, None, fill=fill, coefficients=_PERSPECTIVE_COEFFS[0]) | |
| yield ArgsKwargs(image_loader, startpoints=None, endpoints=None, fill=fill, coefficients=_PERSPECTIVE_COEFFS[0]) |
There was a problem hiding this comment.
Happy to make this change on the tests. I wouldn't want to make it on the main code because the arguments are positional not keyword args. Any concerns on that?
There was a problem hiding this comment.
Nono, fine with that. The test definition is just hard to read without knowledge of the exact signature of the kernel.
There was a problem hiding this comment.
Something, somewhere in the testing framework seems to send multiple parameters to the kernel. I don't want to waste too much time on this, I've reverted the named params change and we can do the optimization on a follow up as it doesn't affect the main implementation or the quality of the tests.
datumbox
changed the title
Summary: * Restore BC on perspective * Fixes linter * Fixing tests. * Apply code-review changes. * Pleasing mypy. * Revert named parameters. Reviewed By: NicolasHug Differential Revision: D41265194 fbshipit-source-id: 4e72d73342d179071458d15593b9cee00eaa84e5
This PR is a potential solution for restoring BC on the perspective kernel.
@vfdev-5 noted that on the new kernels we use the
perspective_coeffsparameter which was favoured on the_FTand_FPstable kernels instead of thestartpointsandendpointsparameters exposed by stableF. This would be BC breaking and thus we have to restore the original parameters.Unfortunately the estimation of
perspective_coeffsis quite computationally expensive and thus it would be great if we can estimate them once per record on theRandomPerspectivetransform and share it with all the inputs. A middle ground is to introduce acoefficientsparameters on all theperspective*kernels and dispatchers which will allow users to pass directly the coefficients if they have them.This PR:
startpointsandendpointsparameters on the original positional locations ofperspective*methods.coefficientsin all theperspective*methods.Nonevalues. Eitherstartpoints/endpointsorcoeffiecientsmust be non-None.startpoints=None, endpoints=Noneandcoeffiecients=perspective_coeffs.Might be worth on a follow up to add tests for the case where
startpoints/endpointsare non-None.cc @vfdev-5 @bjuncek @pmeier