-
Notifications
You must be signed in to change notification settings - Fork 7.1k
port FiveCrop and TenCrop to prototype API #5513
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
💊 CI failures summary and remediationsAs of commit f1a5003 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
return [apply_recursively(fn, item) for item in obj] | ||
elif isinstance(obj, collections.abc.Mapping): | ||
return {key: apply_recursively(fn, item) for key, item in obj.items()} | ||
exclude_sequence_types: Collection[Type] = (str,), |
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.
We need this addition to be able to exclude named tuples as sequences in the BatchMultiCrop
transform. My gut says we are going to need this fine grained control again for other transforms that are not yet ported / implemented. If that turns out not to be true, I'm happy to simply implement a custom solution only in the BatchMultiCrop
transform given that we probably deprecate it anyway.
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.
We should start from the assumption that this is not needed and add it later. Starting with a simple solution first is a good prior. Please simplify as much as possible.
return apply_recursively( | ||
functools.partial(self._transform, params=self._get_params(sample)), | ||
sample, | ||
exclude_sequence_types=(str, *self._MULTI_CROP_TYPES), |
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.
We need this exclude here, because named tuples by default would be recognized as sequence and thus we would only get the individual elements rather than everything at once.
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 that's one more reason not to use named tuples.
Co-authored-by: Philip Meier <[email protected]>
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 As we discussed previously offline, the specific transforms are a bit problematic as they don't fit nicely on the new API.
The only reason why we provide support for them is for BC, so I would avoid introducing any new features. Let's offer the simplest possible solution just to ensure we won't break anyone's code.
return [apply_recursively(fn, item) for item in obj] | ||
elif isinstance(obj, collections.abc.Mapping): | ||
return {key: apply_recursively(fn, item) for key, item in obj.items()} | ||
exclude_sequence_types: Collection[Type] = (str,), |
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.
We should start from the assumption that this is not needed and add it later. Starting with a simple solution first is a good prior. Please simplify as much as possible.
return apply_recursively( | ||
functools.partial(self._transform, params=self._get_params(sample)), | ||
sample, | ||
exclude_sequence_types=(str, *self._MULTI_CROP_TYPES), |
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 that's one more reason not to use named tuples.
After some offline discussion @datumbox and I agreed that we indeed need the |
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.
I think we should also review on the near future whether the very detailed nested sample
is something we need to support or if that's something we don't need on the Datasets. If it turns out it's not needed it will allow us to massively simplify the transforms.
cc @NicolasHug
Summary: * port FiveCrop and TenCrop to prototype API * fix ten crop for pil * Update torchvision/prototype/transforms/_geometry.py * simplify implementation * minor cleanup Reviewed By: vmoens Differential Revision: D34878994 fbshipit-source-id: d1220091f9da4515e831bc44e873041ab33f3ce0 Co-authored-by: Philip Meier <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]>
Both transforms are outliers in the current stable API as discussed in #5500 (comment). Even if we want to deprecate them in favor of a different approach eventually, we need to support them at day 0 of the roll out.
The issues with these transforms arise, because they always need to be followed by another to batch them together:
vision/torchvision/transforms/transforms.py
Lines 980 to 984 in 71d2bb0
We cannot re-use this lambda approach since crops might be buried inside a nested container. Thus, we need a custom transformation that picks up on the result of such a multi crop transform and batches it. To communicate which elements need to be batched between the two transformed, I've opted to wrap the return values of the multi crop kernels in named tuples, because they are full JIT scriptable. The old
FiveCrop
returned a regular tuple, so this is 100% compatible.TenCrop
returned a list, but this is still fine unless someone is using list specific methods like append or the like.Example usage:
To test with PIL images, insert
transforms.Lambda(to_pil_image, features.Image)
after the decoding.