-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Adding A.AtLeastOneBBoxRandomCrop #2207
Adding A.AtLeastOneBBoxRandomCrop #2207
Conversation
…AtLeastOneBBoxRandomCrop.
Reviewer's Guide by SourceryThis PR introduces a new image transformation called Class diagram for AtLeastOneBBoxRandomCropclassDiagram
class AtLeastOneBBoxRandomCrop {
+int height
+int width
+float erosion_factor
+float p
+bool|None always_apply
+get_params_dependent_on_data(params: dict[str, Any], data: dict[str, Any]) dict[str, tuple[int, int, int, int]]
+get_transform_init_args_names() tuple[str, ...]
}
AtLeastOneBBoxRandomCrop --|> BaseCrop
class BaseCrop {
<<abstract>>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @guillaume-rochette-oxb - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding specific unit tests to verify the bounding box retention logic and erosion factor behavior
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Looks good. One small thing. When new transform is added, one need to run: python -m tools.make_transforms_docs make it will output the new table with transforms and targets. You will need to paste it to the readme, instead of the original one. |
Hi, |
@guillaume-rochette-oxb Thank you for the PR. I will update |
Hi,
I'd like to add the transform
AtLeastOneBBoxRandomCrop
to the list of available transforms.What does it do?
It crops an image at a given resolution, similarly to
RandomCrop
. However, unlikeBBoxSafeRandomCrop
, which ensures that all the bounding boxes are retained,AtLeastOneBBoxRandomCrop
will ensure that at least one bounding box is retained.Why?
RandomCrop
, it ensures to keep at least one bounding box, so models, therefore models, such as FasterRCNN or MaskRCNN, which require at least one box to compute the losses. This allow avoiding that "annoying moment", where the training would crash in the event where all bounding boxes would be dropped.BBoxSafeRandomCrop
requires an extra resizing transform in order to align every image to a common resolution, which is beneficial for accelerating training using options like torch.backends.cudnn.benchmark.Feel free to ask further questions about the motivations and/or modifications to in the PR.
Best regards,
Summary by Sourcery
Add the AtLeastOneBBoxRandomCrop transform to ensure at least one bounding box is retained during cropping, enhancing model training stability and efficiency.
New Features:
Tests: