-
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
[proto] Ported RandomIoUCrop from detection refs #6401
Conversation
vfdev-5
commented
Aug 11, 2022
•
edited by pmeier
Loading
edited by pmeier
- Ported RandomIoUCrop transform
- Check visually
- Tests to add
…o proto-random-iou-crop
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.
@pmeier any thoughts?
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.
Small naming nit for consistency. Otherwise LGTM.
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
elif isinstance(output, features.SegmentationMask) and output.shape[-3] > 1: | ||
# apply is_within_crop_area if mask is one-hot encoded | ||
masks = output[is_within_crop_area] | ||
output = features.SegmentationMask.new_like(output, masks) |
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.
@datumbox here is a support for one-hot encoded masks meanwhile other solutions we could decide about
…o proto-random-iou-crop
sample = inputs if len(inputs) > 1 else inputs[0] | ||
if not ( | ||
has_all(sample, features.BoundingBox) | ||
and has_any(sample, PIL.Image.Image, features.Image) |
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.
What about plain tensors? For CutMix
and MixUp
we don't allow the "old" image types at all:
vision/torchvision/prototype/transforms/_augment.py
Lines 108 to 109 in 162267c
if not has_all(sample, features.Image, features.OneHotLabel): | |
raise TypeError(f"{type(self).__name__}() is only defined for Image's *and* OneHotLabel's.") |
Given that have ported this from references there is no BC constraint to allow plain tensors and PIL images. Still, it feels unnecessary restrictive.
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.
To understand your comment, you want to add support for images as Tensors and keep Image and PIL Image ?
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.
Either that or remove support for PIL here. We should either support all image types or only the "new one" like we do in CutMix
and MixUp
(not saying this is a good thing, but we should be consistent).
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.
Is the reason why CutMix, MixUp are not supporting PIL due to lack of implementation ?
Here in RandomIoUCrop we can support everything, so I would add image as torch.Tensor support and keep PIL.
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 we should support all types. Part of the API supporting them and part not is weird and will hinter adoption.
#6417) * port `FixedSizeCrop` from detection references to prototype transforms * mypy * [skip ci] call invalid boxes and corresponding masks and labels * cherry-pick missing functions from #6401 * fix feature wrapping * add test * mypy * add input type restrictions * add test for _get_params * fix input checks
Summary: * [proto] Ported RandomIoUCrop from detection refs * Scope acceptable data types * Added get_params test * Added test__transform_empty_params * Added support for OneHotLabel and tests * Added tests for mask * Updated error message * Apply suggestions from code review * Added support for OHE masks and tests * Ignored mypy error * Fixed forward call on sample * Added a todo Reviewed By: datumbox Differential Revision: D39013670 fbshipit-source-id: 47101bf538c88a5905ab6e5f4c5984271b8c57cd Co-authored-by: Philip Meier <github.pmeier@posteo.de> Co-authored-by: Philip Meier <github.pmeier@posteo.de>
…transforms (#6417) Summary: * port `FixedSizeCrop` from detection references to prototype transforms * mypy * [skip ci] call invalid boxes and corresponding masks and labels * cherry-pick missing functions from #6401 * fix feature wrapping * add test * mypy * add input type restrictions * add test for _get_params * fix input checks Reviewed By: datumbox Differential Revision: D39013661 fbshipit-source-id: cd0d4275c1b2b496745cd1e3af5f35eb5b33fda3