-
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
box_rescale function to rescale bounding boxes to a new image shape #3980
Conversation
Hi @tflahaul! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
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.
Thanks for the PR @tflahaul, welcome to TorchVision!
Since it's your first contribution to the project, I'll just mention a couple of best-practices that maximize your chances of getting your PRs merged:
- When introducing a new major feature, it's often good to create first an issue and provide more information on your use-case. This will help other maintainers give feedback, offer alternatives or help you design the API.
- All new operators and public methods need to be covered by unit-tests, have proper documentation (which you do) and make use of proper typing (which again you do fine).
Given the above, could you add a brief description on your PR to explain what you would like to do? We are in the process of updating our detection transforms so noting your use-case will be helpful.
hey thanks for your feedback @datumbox ! And sorry for the messy PR haha, I'll follow your advices next time. Here's some context: I made this function for my detection script because the model needed square images as input. But the bounding boxes outputted by the model were then based on the squared images and I had to rescale them to their original image sizes before drawing them. It's a very short & simple function but I thought it would be useful to many other. The function multiplies the x and y's by their respective width and height ratios Hope this makes more sense, have a great day! |
@tflahaul No worries! Thanks for the context. TorchVision already has a similar method here: vision/torchvision/models/detection/transform.py Lines 279 to 293 in b060163
It also offers transforms that handle masks, boxes, keypoints etc: vision/torchvision/models/detection/transform.py Lines 150 to 173 in b060163
Perhaps this covers your use-case? |
Oh I had no idea this function existed but it does cover my case, thanks for letting me know! Would it still be interesting to use my implementation for resize_boxes ? It needs a few tweaks but after running some tests I noticed an average speedup of about 28% on my CPU. And from looking at the code I guess using the slices would use a bit less memory too. |
@tflahaul Hmm, it's a tough call on this one... At the moment, we are in the process of reviewing ways to improve the transforms for Detection, so having your use-case in mind is helpful. The problem with adding this PR in is that we already have a way to do the same thing. Our API is far from perfect and that's why we want to improve it but the functionality is already there. I think I'm weighting towards closing this PR and pinging you once we are ready to implement part of the Detection Transform API in case you want to contribute. This will give us time to finalize the API and reduce the BC-breaking changes. Thoughts? BTW if you have time and you would like to contribute to the project have a look for issues tagged with "good first issue" and "help wanted". 😃 |
Yes I understand, this seems like the best thing to do right now. |
@tflahaul Awesome, sounds like a plan! |
Added a small function (+ doc) in the ops module to rescale bounding boxes to a new image shape.
(It's my first time contributing so I hope I'm doing it right)