Move OBB boxes #1350

eric220 · 2024-07-12T14:06:48Z

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

CLAassistant · 2024-07-12T14:06:53Z

All committers have signed the CLA.

LinasKo · 2024-07-12T16:06:03Z

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.

Bhavay-2001 · 2024-07-12T16:43:37Z

Hi @LinasKo, let @eric220 work with this one and you can assign me some other issue. I would be happy to help.
Thanks

LinasKo · 2024-07-12T16:58:47Z

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.

eric220 · 2024-07-12T17:21:05Z

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: ***@***.***>

LinasKo · 2024-07-12T17:31:31Z

Hi @eric220!

@onuralpszr will give some tips in terms of commit management 😉

I'll do a quick code review in a sec

LinasKo · 2024-07-12T17:36:30Z

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.

eric220 · 2024-07-13T15:02:20Z

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: ***@***.***>

LinasKo

@onuralpszr, when you have the time, could you please help us out with a clean way to handle the commit issues?

@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 😉

LinasKo · 2024-07-15T10:12:54Z

supervision/detection/tools/inference_slicer.py

@@ -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.

LinasKo · 2024-07-15T10:13:39Z

supervision/detection/tools/inference_slicer.py

@@ -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=offset


Let's change the variable name from xyss to xyxyxyxy.

eric220 · 2024-07-15T14:44:21Z

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 <#1350 (comment)> : > @@ -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 <#1350 (comment)> : > @@ -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=offset Let's change the variable name from xyss to xyxyxyxy. — Reply to this email directly, view it on GitHub <#1350 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5VRE7V5QI7TA23O4KWCDLZMOOQZAVCNFSM6AAAAABKY75SAGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZXGMZTKMJSHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

onuralpszr · 2024-07-15T14:48:55Z

For commit I will give you fix, I have some stuff to finish ;) wait a little bit

onuralpszr · 2024-07-15T18:10:01Z

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="ericcriteser@Erics-MBP.attlocal.net"
CORRECT_NAME="Eric Criteser"
CORRECT_EMAIL="criteser2@gmail.com"
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^.."

eric220 · 2024-07-15T19:25:29Z

Thank you! I’ll give it a rest now tonight

onuralpszr · 2024-07-16T14:23:27Z

~~@eric220 did you just duplicate commits ?~~

LinasKo · 2024-07-16T14:47:13Z

supervision/detection/tools/inference_slicer.py

@@ -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.

eric220 · 2024-07-16T14:48:37Z

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 <#1350 (comment)> : > @@ -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 <#1350 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5VRE4ZWTKGNQOZROU7WFDZMUXAZAVCNFSM6AAAAABKY75SAGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOBQGQ4TCNJUGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

eric220 · 2024-07-16T14:48:50Z

@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 <#1350 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5VREZKXDU77OOUVXMFLY3ZMUUHNAVCNFSM6AAAAABKY75SAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZRGA2TGNZTHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

LinasKo · 2024-07-16T16:12:56Z

@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 · 2024-07-16T16:36:37Z

@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.

onuralpszr · 2024-07-16T16:39:41Z

@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

eric220 · 2024-07-16T20:45:15Z

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: ***@***.***>

onuralpszr · 2024-07-16T20:46:32Z

@eric220 you are very welcome, no need to make new pr we can continue all good. :)

SkalskiP · 2024-07-17T11:10:52Z

supervision/detection/tools/inference_slicer.py

@@ -4,9 +4,10 @@

 import numpy as np

+from supervision.config import ORIENTED_BOX_COORDINATES as o_b_c


Please import from supervision.config import ORIENTED_BOX_COORDINATES and skip as o_b_c. The name is long but descriptive. It is also consistent with the rest of the codebase.

SkalskiP · 2024-07-17T11:18:02Z

supervision/detection/utils.py

@@ -596,6 +596,44 @@ def move_boxes(
    return xyxy + np.hstack([offset, offset])


+def move_obb_boxes(


Please rename the function to move_oriented_boxes.

SkalskiP · 2024-07-17T11:31:56Z

supervision/detection/utils.py

+        #[45., 50.]]]
+        ```
+    """
+    return [


I just wanted to let you know that you don't need to do it like this. Numpy supports broadcasting. Broadcasting allows NumPy to perform element-wise operations on arrays of different shapes by automatically expanding the smaller array to match the shape of the larger one.

You can simply xyxyxyxy + offset.

You can learn more from this colab: https://colab.research.google.com/drive/10FVlOMQaStrg7yUo-_x4l_NEBafHzUqI?usp=sharing

SkalskiP · 2024-07-17T11:40:02Z

supervision/detection/utils.py

+def move_obb_boxes(
+    xyxyxyxy: npt.NDArray[np.float64], offset: npt.NDArray[np.int32]
+) -> npt.NDArray[np.float64]:
+    """


To make docsting consistent with rest of the codebase, please replace it with:

""" Parameters: xyxyxyxy (npt.NDArray[np.float64]): An array of shape `(n, 4, 2)` containing the oriented bounding boxes coordinates in format `[[x1, y1], [x2, y2], [x3, y3], [x3, y3]]` offset (np.array): An array of shape `(2,)` containing offset values in format is `[dx, dy]`. Returns: npt.NDArray[np.float64]: Repositioned bounding boxes. Examples: ```python import numpy as np import supervision as sv xyxyxyxy = np.array([ [ [20, 10], [10, 20], [20, 30], [30, 20] ], [ [30 ,30], [20, 40], [30, 50], [40, 40] ] ]) offset = np.array([5, 5]) sv.move_oriented_boxes(xyxy=xyxy, offset=offset) # array([ # [ # [25, 15], # [15, 25], # [25, 35], # [35, 25] # ], # [ # [35, 35], # [25, 45], # [35, 55], # [45, 45] # ] # ]) ``` """

SkalskiP · 2024-07-17T11:41:35Z

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.

…sing broadcasting, change docstring

eric220 · 2024-07-17T13:27:46Z

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: ***@***.***>

SkalskiP · 2024-07-17T16:22:18Z

Thanks a lot @eric220 🙏🏻 I just asked here #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! 🔥

SkalskiP · 2024-07-18T13:43:48Z

Looks like tests went well: #1339 (comment). Merging!

eric220 added 5 commits July 11, 2024 13:38

added functionality for oriented bounding boxes

60d5660

Run pre-commit and fix issues

7fa7907

correct check to detections.data

9d5a477

change move_obb_boxesto accept array of arrays

377ca27

change move_obb_boxes to accept multiple detections

113d4b6

eric220 added 2 commits July 12, 2024 09:26

added email to the push

9a61b3c

small change to comments, added email to submission

cea854e

eric220 and others added 2 commits July 13, 2024 08:18

commit under new branch

c9d4f2b

fix(pre_commit): 🎨 auto format pre-commit hooks

5ee8fcf

LinasKo suggested changes Jul 15, 2024

View reviewed changes

eric220 and others added 3 commits July 16, 2024 08:47

added email to the push

c71d945

update entire methods: move_obb_boxes and move detections

2544d72

fix(pre_commit): 🎨 auto format pre-commit hooks

48f6c8d

LinasKo suggested changes Jul 16, 2024

View reviewed changes

change import config to import config.OREIENTED_BOX_COORDINATES

9b0568c

onuralpszr force-pushed the develop branch from d7d6c06 to 9b0568c Compare July 16, 2024 16:35

SkalskiP requested changes Jul 17, 2024

View reviewed changes

eliminate alias obc, rename function to move_oriented_boxes, return u…

b21a478

…sing broadcasting, change docstring

SkalskiP approved these changes Jul 18, 2024

View reviewed changes

SkalskiP merged commit 7ca5b01 into roboflow:develop Jul 18, 2024
9 checks passed

SkalskiP mentioned this pull request Jul 23, 2024

Clarification of obb behaviour with InferenceSlicer #1394

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move OBB boxes #1350

Move OBB boxes #1350

eric220 commented Jul 12, 2024

CLAassistant commented Jul 12, 2024 •

edited

Loading

LinasKo commented Jul 12, 2024 •

edited

Loading

Bhavay-2001 commented Jul 12, 2024

LinasKo commented Jul 12, 2024 •

edited

Loading

eric220 commented Jul 12, 2024 via email

LinasKo commented Jul 12, 2024 •

edited

Loading

LinasKo commented Jul 12, 2024 •

edited

Loading

eric220 commented Jul 13, 2024 via email

LinasKo left a comment

LinasKo Jul 15, 2024

LinasKo Jul 15, 2024

eric220 commented Jul 15, 2024 via email

onuralpszr commented Jul 15, 2024 •

edited

Loading

onuralpszr commented Jul 15, 2024 •

edited

Loading

eric220 commented Jul 15, 2024 via email •

edited by onuralpszr

Loading

onuralpszr commented Jul 16, 2024 •

edited

Loading

LinasKo Jul 16, 2024

eric220 commented Jul 16, 2024 via email

eric220 commented Jul 16, 2024 via email

LinasKo commented Jul 16, 2024

onuralpszr commented Jul 16, 2024

onuralpszr commented Jul 16, 2024 •

edited

Loading

eric220 commented Jul 16, 2024 via email

onuralpszr commented Jul 16, 2024

SkalskiP Jul 17, 2024

SkalskiP Jul 17, 2024

SkalskiP Jul 17, 2024

SkalskiP Jul 17, 2024

SkalskiP commented Jul 17, 2024

eric220 commented Jul 17, 2024 via email

SkalskiP commented Jul 17, 2024

SkalskiP commented Jul 18, 2024

		@@ -4,9 +4,10 @@

		import numpy as np

		from supervision.config import ORIENTED_BOX_COORDINATES as o_b_c

		@@ -596,6 +596,44 @@ def move_boxes(
		return xyxy + np.hstack([offset, offset])


		def move_obb_boxes(

Move OBB boxes #1350

Move OBB boxes #1350

Conversation

eric220 commented Jul 12, 2024

Description

How has this change been tested, please provide a testcase or example of how you tested the change?

Docs

CLAassistant commented Jul 12, 2024 • edited Loading

LinasKo commented Jul 12, 2024 • edited Loading

Bhavay-2001 commented Jul 12, 2024

LinasKo commented Jul 12, 2024 • edited Loading

eric220 commented Jul 12, 2024 via email

LinasKo commented Jul 12, 2024 • edited Loading

LinasKo commented Jul 12, 2024 • edited Loading

eric220 commented Jul 13, 2024 via email

LinasKo left a comment

Choose a reason for hiding this comment

LinasKo Jul 15, 2024

Choose a reason for hiding this comment

LinasKo Jul 15, 2024

Choose a reason for hiding this comment

eric220 commented Jul 15, 2024 via email

onuralpszr commented Jul 15, 2024 • edited Loading

onuralpszr commented Jul 15, 2024 • edited Loading

eric220 commented Jul 15, 2024 via email • edited by onuralpszr Loading

onuralpszr commented Jul 16, 2024 • edited Loading

LinasKo Jul 16, 2024

Choose a reason for hiding this comment

eric220 commented Jul 16, 2024 via email

eric220 commented Jul 16, 2024 via email

LinasKo commented Jul 16, 2024

onuralpszr commented Jul 16, 2024

onuralpszr commented Jul 16, 2024 • edited Loading

eric220 commented Jul 16, 2024 via email

onuralpszr commented Jul 16, 2024

SkalskiP Jul 17, 2024

Choose a reason for hiding this comment

SkalskiP Jul 17, 2024

Choose a reason for hiding this comment

SkalskiP Jul 17, 2024

Choose a reason for hiding this comment

SkalskiP Jul 17, 2024

Choose a reason for hiding this comment

SkalskiP commented Jul 17, 2024

eric220 commented Jul 17, 2024 via email

SkalskiP commented Jul 17, 2024

SkalskiP commented Jul 18, 2024

CLAassistant commented Jul 12, 2024 •

edited

Loading

LinasKo commented Jul 12, 2024 •

edited

Loading

LinasKo commented Jul 12, 2024 •

edited

Loading

LinasKo commented Jul 12, 2024 •

edited

Loading

LinasKo commented Jul 12, 2024 •

edited

Loading

onuralpszr commented Jul 15, 2024 •

edited

Loading

onuralpszr commented Jul 15, 2024 •

edited

Loading

eric220 commented Jul 15, 2024 via email •

edited by onuralpszr

Loading

onuralpszr commented Jul 16, 2024 •

edited

Loading

onuralpszr commented Jul 16, 2024 •

edited

Loading