Added shift method #9

harshraj22 · 2022-06-10T11:34:36Z

Linked with issue #5

All bounding box types convert to a VocBoundingBox object. The shifting is done in the VOC coordinates, and a new object of same class is being returned. All other properties are kept as passed during the initialisation of the current object.

devrimcavusoglu · 2022-06-10T13:06:31Z

Thanks for the PR @harshraj22 👍 I have one minor comment, that we can accept a tuple of ints as Tuple[int, int] for shift() method, and we can give the description in the docstring like (horizontal_shift, vertical_shift), it'd be better suited for future (as potential 3d boxes implementations). It can be like the following, wdyt ?

def shift(self, shift_amount):
    horizontal_shift, vertical_shift = shift_amount
    ...

Also, can you add test cases for the new method shift() ?

devrimcavusoglu · 2022-06-10T13:15:38Z

pybboxes/types/albumentations_bounding_box.py

@@ -34,6 +34,28 @@ def to_voc(self, return_values: bool = False) -> Union[Tuple[int, int, int, int]
            return x_tl, y_tl, x_br, y_br
        return BoundingBox(x_tl, y_tl, x_br, y_br, image_size=self.image_size, strict=self.strict)

+    def shift(self, horizontal_threshold: float, vertical_threshold: float) -> "AlbumentationsBoundingBox":


Please, take this comment as for other methods as well.

(1) Since we convert to VOC, the type hints as well as the docstring needs to be changed, only integers are accepted since we use VOC type under the hood for shifting.
(2) We need to clearly desribe and explicitly write in the docstring that shifting is performed in pixel values not in normalized values.

@devrimcavusoglu I did not understand what do you mean by "pixel values not in normalised values". Could you clarify a little more ?

Sure. I meant unnormalized values, like those shift values (horizontal_threshold, vertical_threshold) must be always in pixel values integers because VOC style is in pixel values (x,y coordinates). For example YOLO style is (x-center, y-center, width, height) but these values are normalized (w.r.t image_size) values and not pixel coordinates.

Currently, in the implementation in this PR, regardless of the box style the box is first converted to VOC style, and addition is then applied. That's why, for example, we cannot give normalized shift values to YOLO style values (normalized). Alternatively, if we take the input shift values in that particular box style, then VOC style conversion must be omitted. Further, see the pseudo snippet below

yolo_values = [0.4046875, 0.840625, 0.503125, 0.24375] yolo_bbox = BoundingBox.from_yolo(*yolo_values, image_size=(640,480)) # I want to shift my YOLO box shifted_yolo_box = yolo_bbox.shift(0.07, 0.02)

Now, when I call .shift() internally these steps happen; (1) yolo_values are converted to voc values, (2) my shift values are added to the associated values 0.07, 0.02 respectively.

At step (2) yolo shift values 0.07, 0.02 are added to VOC values which are [98, 345, 420, 462] which is not desired. There are two solutions that comes up,

Solution 1 (Current)

Accept shift values as unnormalized. Convert the box values to voc, add the shift values, and return.

Solution 2

Omit converting values to VOC, directly apply the shift values and then instantiate/construct the object and return.

Both solutions are fine, and actually I guided you towards solution 1 although currently when I consider the case again, solution 2 seems a better alternative. In this case, I kinda misguided you towards solution 1. Thus, I see currently two options,

Keep solution 1 (current implementation), but fix the accept type (all should be int because of converting VOC) and also add an info to the docstring about shift values must be unnormalized.

Switch to solution 2 (might be better).

@harshraj22 wdyt of these alternatives ?

Yes, I too would prefer solution 2. So if I understand correctly, it would be overwriting shift method in each BoundingBox type.

Yes, I too would prefer solution 2. So if I understand correctly, it would be overwriting shift method in each BoundingBox type.

Your implementation so far will remain as is except the to_voc conversion at step 1 since we'd like to update the values associated with each style. In short, simply remove to_voc parts in your implementation and all is fine. Also, your return part changes directly as cls() instead of cls.from_voc()

Well, in your fork for YOLO box everything seems fine, with the code snippet below, I got the desired output:

>>> yolo_values = [0.4046875, 0.840625, 0.503125, 0.24375] >>> yolo_bbox = YoloBoundingBox.from_array(yolo_values, image_size=(640, 480)) >>> shifted_yolo_bbox = yolo_bbox.shift((0.08, 0.03)) >>> print(yolo_bbox) >>> print(shifted_yolo_bbox) <[0.4047 0.8406 0.5031 0.2437] (322x117) | Image: (640x480)> <[0.4847 0.8706 0.5031 0.2437] (322x117) | Image: (640x480)>

,and this

>>> oob_bbox = yolo_bbox.shift((0.08, 0.2)) >>> print(oob_bbox) Traceback (most recent call last): ... ValueError: Given bounding box values is out of bounds.

All in all, everything seems fine. Can you give minimal example to reproduce your case ? Could you be accidentally shifted the box out of bounds ? @harshraj22 and can you give platform info, what OS and version, python version, etc.

That's strange ! macOS Monterey, python3.8.9
to reproduce, from the latest commit on my branch, run pytest from the pytest directory.

Hi @harshraj22, sorry for late reply. Well, I've checked it. The tests failing simply because the shift amount tries to shift the boxes out of bounds,

# Albumentations box <[0.1531 0.7188 0.6562 0.9625] (322x117) | Image: (640x480)> # the shift amount (2, 2)

Albumentations, fiftyone and YOLO are bounding box styles with normalized values meaning that the bounding boxes that are not OOB needs to be in (0,1) range, clearly with the shift (2,2) they are pushed towards OOB. My advise is that, define two distinct fixtures for normalized shift, and unnormalized shift and apply accordingly.

Also, test case for COCO is failing due to mismatch between actual being list, and desired being tuple, simply just match the types either make both lists or both tuples, make them consistent among all test cases so chose one type for all test cases.

The shift( ) method takes Tuple[int, int] as arg right ? We can't pass a tuple of values in (0, 1) ?

The shift( ) method takes Tuple[int, int] as arg right ? We can't pass a tuple of values in (0, 1) ?

No, we've picked the second option (see Solution 2), and thus on shift() method every box takes their associated values. That is, normalized box styles (Albumentations, Fiftyone, YOLO) will take floats between (0,1), and unnormalized box styles (COCO, VOC) will take integers.

harshraj22 · 2022-06-13T05:59:43Z

Test script specified in readme fails. Any idea why ?

devrimcavusoglu · 2022-06-13T08:33:46Z

Test script specified in readme fails. Any idea why ?

Yes, it is missing in the readme (I'll add). You have to install like

pip install -e .[dev]

Afterwards, the test cases should be running without any problem.

devrimcavusoglu · 2022-06-18T08:14:22Z

@harshraj22 sorry for late review. Thanks for the effort, I think we are almost there. I have two major comments:

Let's rename threshold to amount as "threshold" may be misleading. Also, after this change please update the docstrings accordingly.
shift() is missing for bbox.BoundingBox

Additionaly, there are some conflicts due to the release 0.1.1, can you please solve the conflicts also.

devrimcavusoglu · 2022-06-16T20:08:04Z

pybboxes/types/albumentations_bounding_box.py

@@ -1,5 +1,7 @@
 from typing import Tuple, Union

+from cv2 import threshold


Unused import should be removed

devrimcavusoglu · 2022-06-18T08:11:06Z

pybboxes/types/coco_bounding_box.py

@@ -3,6 +3,8 @@
 from pybboxes.types.base import BaseBoundingBox
 from pybboxes.types.bbox import BoundingBox

+# from pybboxes.functional import convert_bbox


Remove unused import.

devrimcavusoglu · 2022-06-18T08:15:48Z

pybboxes/types/albumentations_bounding_box.py

@@ -34,6 +36,35 @@ def to_voc(self, return_values: bool = False) -> Union[Tuple[int, int, int, int]
            return x_tl, y_tl, x_br, y_br
        return BoundingBox(x_tl, y_tl, x_br, y_br, image_size=self.image_size, strict=self.strict)

+    def shift(self, threshold: Tuple[int, int]) -> "AlbumentationsBoundingBox":


Tuple[float,float] for Albumentations style, update the docstring also.

Added shift method to box.BoundingBox

harshraj22 · 2022-06-20T14:09:05Z

@devrimcavusoglu Made the required changes. Please review again :)

devrimcavusoglu · 2022-06-20T19:00:49Z

@devrimcavusoglu Made the required changes. Please review again :)

Thanks for the effort appreciated 🚀 I'll run the tests now though I have a single minor comment/suggestion in one naming.

Edit: Tests are failing due to formatting, please see code-style guide.

devrimcavusoglu · 2022-06-20T19:05:16Z

pybboxes/types/albumentations_bounding_box.py

+        AlbumentationsBoundingBox
+            The new bounding box.
+        """
+        horizontal_threshold, vertical_threshold = amount


Can we rename these also (take this comment into consideration for all box types please)

horizontal_threshold, vertical_threshold -> horizontal_shift, vertical_shift

* Formatted code * Passes tests

Added shift method

e705822

harshraj22 force-pushed the shift-scale branch from 7e65eaa to e705822 Compare June 10, 2022 11:39

devrimcavusoglu reviewed Jun 10, 2022

View reviewed changes

harshraj22 added 3 commits June 13, 2022 11:10

Updated the args for shift method

4cd14c8

Added tests for shift method

9c54dbf

Auto code refactoring by Black

fa02428

Removed unnecessary auto import

362ae61

harshraj22 force-pushed the shift-scale branch from c7987c0 to 362ae61 Compare June 13, 2022 06:02

harshraj22 marked this pull request as draft June 13, 2022 06:03

harshraj22 added 3 commits June 13, 2022 21:45

[Bug] Trying to fix shift method implementation

2660140

Added tests for shift method

0461e53

formatting by Black

dc89694

harshraj22 marked this pull request as ready for review June 16, 2022 09:37

harshraj22 requested a review from devrimcavusoglu June 16, 2022 09:37

devrimcavusoglu reviewed Jun 18, 2022

View reviewed changes

harshraj22 added 2 commits June 20, 2022 19:36

Renamed args of shift method

a89829a

Added shift method to box.BoundingBox

Merge branch 'main' into shift-scale

3c50c88

harshraj22 requested a review from devrimcavusoglu June 20, 2022 14:09

devrimcavusoglu reviewed Jun 20, 2022

View reviewed changes

Renamed variables

7d707d1

* Formatted code * Passes tests

harshraj22 requested a review from devrimcavusoglu June 21, 2022 04:25

devrimcavusoglu approved these changes Jun 21, 2022

View reviewed changes

devrimcavusoglu merged commit 4abdab9 into devrimcavusoglu:main Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added shift method #9

Added shift method #9

harshraj22 commented Jun 10, 2022

devrimcavusoglu commented Jun 10, 2022

devrimcavusoglu Jun 10, 2022

harshraj22 Jun 13, 2022 •

edited

Loading

devrimcavusoglu Jun 13, 2022

harshraj22 Jun 13, 2022

devrimcavusoglu Jun 13, 2022 •

edited

Loading

devrimcavusoglu Jun 13, 2022 •

edited

Loading

harshraj22 Jun 14, 2022

devrimcavusoglu Jun 15, 2022

harshraj22 Jun 16, 2022

devrimcavusoglu Jun 16, 2022

harshraj22 commented Jun 13, 2022

devrimcavusoglu commented Jun 13, 2022 •

edited

Loading

devrimcavusoglu commented Jun 18, 2022 •

edited

Loading

devrimcavusoglu Jun 16, 2022

devrimcavusoglu Jun 18, 2022

devrimcavusoglu Jun 18, 2022

harshraj22 commented Jun 20, 2022

devrimcavusoglu commented Jun 20, 2022 •

edited

Loading

devrimcavusoglu Jun 20, 2022

harshraj22 Jun 21, 2022

		@@ -1,5 +1,7 @@
		from typing import Tuple, Union

		from cv2 import threshold

Added shift method #9

Added shift method #9

Conversation

harshraj22 commented Jun 10, 2022

devrimcavusoglu commented Jun 10, 2022

Choose a reason for hiding this comment

harshraj22 Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Solution 1 (Current)

Solution 2

Choose a reason for hiding this comment

devrimcavusoglu Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

devrimcavusoglu Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshraj22 commented Jun 13, 2022

devrimcavusoglu commented Jun 13, 2022 • edited Loading

devrimcavusoglu commented Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshraj22 commented Jun 20, 2022

devrimcavusoglu commented Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshraj22 Jun 13, 2022 •

edited

Loading

devrimcavusoglu Jun 13, 2022 •

edited

Loading

devrimcavusoglu Jun 13, 2022 •

edited

Loading

devrimcavusoglu commented Jun 13, 2022 •

edited

Loading

devrimcavusoglu commented Jun 18, 2022 •

edited

Loading

devrimcavusoglu commented Jun 20, 2022 •

edited

Loading