-
Notifications
You must be signed in to change notification settings - Fork 7.1k
add support for BoundinBox and SegmentationMask to RandomResizeCrop #6041
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
@pmeier As discussed previously, the example you provide at #5487 (comment) is not a valid real-world use-case. Aka resizing just the bbox without the image will lead to corrupted information. Hence requiring always to have the image in such transforms makes sense. If you manage to find another valid counter example, I'm happy to revise. But unless we have that, there is no need to create a solution that covers for a corner-case that is not needed. Concerning the failure, I think it's a library is missing:
A potentially easy fix is to install the dependencies that probably are missing. See here. |
I agree for vision/torchvision/prototype/transforms/_geometry.py Lines 388 to 390 in edb7bbb
If
Err, that is not what I meant. This is a |
It's worth noting that you are using Pad like that to circumvent a design limitation of the API. You are not using it because Padding a bbox on its own makes sense but rather as a workaround. I think all these are indications that the proposal can be improved. My concern about the use of "primitive transforms" is that I don't know exactly what's their difference with the kernels rather than adding dispatching. Perhaps the whole thing can be solved by moving the dispatch on _Feature objects. |
Re test failures: the missing dependency only happens on Linux. I've opened pytorch/data#418 to check if this is a mistake on their side. The real failures are visible on Windows and macOS. |
@pmeier Thanks, yes I've seen it. I don't remember all the details TBH but I think you could go around the issue by dispatching to the low-level kernel with our normal ifs dispatch approach, right? |
@@ -20,7 +20,7 @@ def fn( | |||
try: | |||
return next(query_recursively(fn, sample))[1] | |||
except StopIteration: | |||
raise TypeError("No image was found in the sample") | |||
raise TypeError("No image was found in the sample") from None |
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.
Why do you add "from None" here ?
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.
Sorry, forgot to add a comment. The StopIteration
is an internal detail that does not need to be propagated outside. By adding from None
the error will look like a normal exception
try:
next(iter([]))
except StopIteration:
raise TypeError("Argh!") from None
TypeError: Argh!
Not doing that gives
try:
next(iter([]))
except StopIteration:
raise TypeError("Argh!")
StopIteration
During handling of the above exception, another exception occurred:
[...]
TypeError: Argh!
Not without duplicating all the other logic that happens in vision/torchvision/prototype/transforms/_geometry.py Lines 313 to 350 in edb7bbb
True. Let's postpone this discussion until we have decided on a general way to handle composite transforms. If we for example reinstate the high-level dispatchers, my argument is moot. Otherwise it might still apply. |
Kernels were already there, so this only adds them to the transform.
Test failures are real though. @datumbox we get bitten now but what I mentioned in #5487 (comment): although the transformation is perfectly able to work with only bounding boxes or segmentation masks, our choice of only extracting the image size from images, forces the user to always include an image in the inputs.