supervision
supervision copied to clipboard
roboflow
Move OBB boxes
Description
Issue: draw wrong OrientedBoxAnnotator with InferenceSlicer #1339 Resolution: create function *utils:move_obb_boxes (as suggested by LinasKo), with a call inside *tools/inference_slicer:move_detections
List any dependencies that are required for this change. No new dependencies required
Please delete options that are not relevant.
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
How has this change been tested, please provide a testcase or example of how you tested the change?
created detection-like object (single and multiple detections) based on the submitted issue, passed to the function and verified results
Docs
I did not change any documentation
Hi @eric220 👋
Thank you for submitting the PR! We need all contributors to sign the CLA before it can be reviewed. (Note that there's been issues with CLA since a few minutes ago. If you signed it already, it could be a system fault / new update we're unfamiliar with.)
Also, as the issue was assigned to @Bhavay-2001, if he opens a PR in a week from now, his code will have priority.
Otherwise, this looks pretty good, except that we don't want xyxyxyxy as the key - there's a specific enum value we're using.
Hi @LinasKo, let @eric220 work with this one and you can assign me some other issue. I would be happy to help. Thanks
Cool. @eric220, please have a look at the CLA. I assume you used 2 different emails in your commits, but let me know if something doesn't work.
Hi @LinasKo! It’s worse the that, it was submitted without an email…Couple of questions: Can you override the CLA requirements this once?Do you have suggestions for modifying the author/committer? I tried -amend and rebase, but both have not produced the desired results. Could I rebranch and resubmit?Could you point me to the enumeration variable (haven’t looked as I am at work, it may be obvious).Kindly,EricSent from my iPhoneOn Jul 12, 2024, at 11:59 AM, Linas Kondrackis @.***> wrote: Cool. @eric220, please have a look at the CLA. I assume you used 2 different emails in your commits.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
Hi @eric220!
@onuralpszr will give some tips in terms of commit management 😉
I'll do a quick code review in a sec
The enum you're looking for is ORIENTED_BOX_COORDINATES.
Note: Please share a Google Colab with minimal code to test the new feature. We know it's additional work, but it will speed up the review process. The reviewer must test each change. Setting up a local environment to do this is time-consuming. Please ensure that Google Colab can be accessed without any issues (make it public). Thank you! 🙏🏻
Here's some models that might be helpful.
Quick update: coding is done and colab built. Still working on a clean branch. I created a new branch, but from my old one. So it still has my split personality as a contributor… Thank you for your patience. EricSent from my iPhoneOn Jul 12, 2024, at 12:36 PM, Linas Kondrackis @.***> wrote: The enum you're looking for is ORIENTED_BOX_COORDINATES. Note: Please share a Google Colab with minimal code to test the new feature. We know it's additional work, but it will speed up the review process. The reviewer must test each change. Setting up a local environment to do this is time-consuming. Please ensure that Google Colab can be accessed without any issues (make it public). Thank you! 🙏🏻
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
Good morning @LinasKo. I am sorry for all the trouble getting this done. Here is the link to the colab: https://colab.research.google.com/drive/1Um94t44u-al-YAmt-lwpe4sSi7jLfWBu?usp=sharing
Regarding fixing my repository; I am thinking of the nuclear option, delete my local and remote and start over with a clean fork etc. Will this mess up your PRs, or anything else on your end in any way?
Also, while I am fixing my repository, please feel free to have someone else copy, paste and submit the code.
On Mon, Jul 15, 2024 at 5:16 AM Linas Kondrackis @.***> wrote:
@.**** requested changes on this pull request.
@onuralpszr https://github.com/onuralpszr, when you have the time, could you please help us out with a clean way to handle the commit issues?
@eric220 https://github.com/eric220, I've made some comments of the code. When you're ready, please post the Colab link here so we can test it too 😉
In supervision/detection/tools/inference_slicer.py https://github.com/roboflow/supervision/pull/1350#discussion_r1677599845 :
@@ -28,6 +28,10 @@ def move_detections( (sv.Detections) repositioned Detections object. """ detections.xyxy = move_boxes(xyxy=detections.xyxy, offset=offset)
- if "ORIENTED_BOX_COORDINATES" in detections.data:
Rather than a string, this needs to be imported from supervision.config.
In supervision/detection/tools/inference_slicer.py https://github.com/roboflow/supervision/pull/1350#discussion_r1677600626 :
@@ -28,6 +28,10 @@ def move_detections( (sv.Detections) repositioned Detections object. """ detections.xyxy = move_boxes(xyxy=detections.xyxy, offset=offset)
- if "ORIENTED_BOX_COORDINATES" in detections.data:
detections.data["ORIENTED_BOX_COORDINATES"] = move_obb_boxes(xyss=detections.data["ORIENTED_BOX_COORDINATES"], offset=offsetLet's change the variable name from xyss to xyxyxyxy.
— Reply to this email directly, view it on GitHub https://github.com/roboflow/supervision/pull/1350#pullrequestreview-2177335128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5VRE7V5QI7TA23O4KWCDLZMOOQZAVCNFSM6AAAAABKY75SAGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZXGMZTKMJSHA . You are receiving this because you were mentioned.Message ID: @.***>
For commit I will give you fix, I have some stuff to finish ;) wait a little bit
Hlelo @LinasKo @eric220
I looked that commits and did a test run in clone of this branch and this should re-write history of problematic commit author. Before push here please also try in clone of your branch and push and check it. If you find issue let me know (on my test it was fine but please re-do so)
That "commit sha" at the end also I used from your branch commit sha when I executed it should only change 7 out 9 commits and you will see it.
git filter-branch --env-filter '
OLD_EMAIL="[email protected]"
CORRECT_NAME="Eric Criteser"
CORRECT_EMAIL="[email protected]"
if [ "$GIT_COMMITTER_EMAIL" = "$OLD_EMAIL" ]
then
export GIT_COMMITTER_NAME="$CORRECT_NAME"
export GIT_COMMITTER_EMAIL="$CORRECT_EMAIL"
fi
if [ "$GIT_AUTHOR_EMAIL" = "$OLD_EMAIL" ]
then
export GIT_AUTHOR_NAME="$CORRECT_NAME"
export GIT_AUTHOR_EMAIL="$CORRECT_EMAIL"
fi
' --tag-name-filter cat -- --branches --tags "d7bac3dc87484a3362473b5a81031b5a47b9101e^.."
Will do
On Tue, Jul 16, 2024 at 9:47 AM Linas Kondrackis @.***> wrote:
@.**** requested changes on this pull request.
In supervision/detection/tools/inference_slicer.py https://github.com/roboflow/supervision/pull/1350#discussion_r1679555005 :
@@ -9,6 +9,7 @@ from supervision.detection.utils import move_boxes, move_masks, move_obb_boxes from supervision.utils.image import crop_image from supervision.utils.internal import SupervisionWarnings +from supervision import config
Let's only import ORIENTED_BOX_COORDINATES, rather than the whole config. This matches the pattern in the rest of the repo.
— Reply to this email directly, view it on GitHub https://github.com/roboflow/supervision/pull/1350#pullrequestreview-2180491544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5VRE4ZWTKGNQOZROU7WFDZMUXAZAVCNFSM6AAAAABKY75SAGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOBQGQ4TCNJUGQ . You are receiving this because you were mentioned.Message ID: @.***>
@onuralp Probably. I am still trying to fix this author/commit issue (I think this is what happened). Last night I ran the script you provided, it fixed it locally, then pushed. I did a pull this morning, The dev branch did not have the code changes, changed the code and pushed. Then realized I didn't run the pre commit checks, ran it and pushed again. Now when I git log, the author/commit has reverted back to the original (incorrect).
On Tue, Jul 16, 2024 at 9:23 AM Onuralp SEZER @.***> wrote:
@eric220 https://github.com/eric220 did you just duplicate commits ?
— Reply to this email directly, view it on GitHub https://github.com/roboflow/supervision/pull/1350#issuecomment-2231053739, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5VREZKXDU77OOUVXMFLY3ZMUUHNAVCNFSM6AAAAABKY75SAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZRGA2TGNZTHE . You are receiving this because you were mentioned.Message ID: @.***>
@onuralpszr, if it's not a trivial fix, let's do a clean submission in a new PR. Beyond a certain point it won't be worth trying to fix it 'correctly' :)
@onuralpszr, if it's not a trivial fix, let's do a clean submission in a new PR. Beyond a certain point it won't be worth trying to fix it 'correctly' :)
All fixed.
@eric220 please check your .gitconfig and git settings carefully and you need re-pull or just do "re-clone" fork of your repo again. I made sure all of them is your correct registered github e-mail and name you used in commits. But one advice If you wanted to, please check github document and how to use gpg with github and enable your gpg-sign and sign off mode as well. (vigilant mode in github) It will gives you more security and control over your github/commits as well.
Docs : https://docs.github.com/en/authentication/managing-commit-signature-verification/displaying-verification-statuses-for-all-of-your-commits Docs2: https://docs.github.com/en/authentication/managing-commit-signature-verification/adding-a-gpg-key-to-your-github-account
Thank you for all your help! I have re-cloned and everything looks good. I am sorry for all the trouble for such a simple fix. I will look at the docs and into using gpg keys. Should I initiate a new PR?Sent from my iPhoneOn Jul 16, 2024, at 11:40 AM, Onuralp SEZER @.***> wrote: @eric220 please check your .gitconfig and git settings carefully and you need re-pull or just do "re-clone" fork repo again. I made sure all of them is your correct registered github e-mail and name you used in commits. Normally I would not do such a thing but one more advice please check github document and please learn how to use gpg with github and enable your gpg-sign and sign off mode as well. (vigilant mode in github) Docs : https://docs.github.com/en/authentication/managing-commit-signature-verification/displaying-verification-statuses-for-all-of-your-commits Docs2: https://docs.github.com/en/authentication/managing-commit-signature-verification/adding-a-gpg-key-to-your-github-account
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
@eric220 you are very welcome, no need to make new pr we can continue all good. :)
Hi @eric220 👋🏻 I'm covering for @LinasKo as he is on vacation. I left a few comments. Let me know if you have any questions.
Done!Sent from my iPhoneOn Jul 17, 2024, at 6:41 AM, Piotr Skalski @.***> wrote: Hi @eric220 👋🏻 I'm covering for @LinasKo as he is on vacation. I left a few comments. Let me know if you have any questions.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
Thanks a lot @eric220 🙏🏻 I just asked here https://github.com/roboflow/supervision/issues/1339 to test this change. @eric220 you can also create a colab testing this change end-to-end (on actual OBB inference).
Once we confirm it works, we can merge! 🔥
Looks like tests went well: https://github.com/roboflow/supervision/issues/1339#issuecomment-2235817171. Merging!