Skip to content
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

Labelling issue and Bounding box drawing issue. #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shreyasvedpathak
Copy link

Issue #5: Fall back to regular labelling if T shaped labels or flag labels are out of frame.
Description: I used the y_top field to find if the label is going out of frame or not. If it is going, i.e. y_top is less than 0, then fall back and draw using the regular labelling method. I had to introduce a new variable in order to keep track of the top of the line in case of the T styled label.

Issue #3: Handle bounding boxes on the edge properly.
Description: I added a function that checks if the given coordinates in the list bbox are coming under the scope of the image. If they are then we return those coordinates, otherwise, we trim them using criteria. Those criteria are described in function docstring of function check_and_modify_bbox(), it can be found in the file bbox_visualizer.py line number 3. I also added a margin argument to this function, which will preserve some space between the box and the image's edge if the user wants it that way. It is set to 0 by default. Finally, I changed the label to be inside the bounding box if the bounding box was the same size as the picture. This ensures that the label is displayed at all times. I modified if the condition on line 115 to execute this functionality.

@shreyasvedpathak
Copy link
Author

I added this feature request (#9) (made by me lol).

Issue: A single function to draw detections with labelling style as a parameter.

Description:
As explained in the feature request description, I added a function (line 495) that will draw detections in a single line. It has a parameter labelling_method which will decide the labelling style. This function works considering issues #5 and #3. I also added a feature that will assign a unique colour to each label (this was not implemented earlier) at line 476. The colour map (the unique colour associated with the label, a dictionary) can also be provided by the user in the draw_detections() function as an argument.

Misc:

  • Added new tests
  • Image at tests/test_detections/cats_with_t_bbox.jpg, demonstrates how fallback works and a different colour for each label.

Copy link
Owner

@shoumikchow shoumikchow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for the PR. Great job on solving #3 and #5. I love the code and the solutions. There are a few changes suggested inline that would be needed before merging.

I'd also move all the test images in the images folder in a separate folder there to make it tidier. This would mean the test files would also need to be updated to reflect the new path.

As mentioned in #9, I'd prefer to not have the draw_detections method at all.

The code needs to pass flake8. Please follow the guidelines given in the CONTRIBUTING page. Since you have added tests, those files would need to pass flake8 too.

If you make these changes, which are fairly minor, I'd be happy to merge the PR.

Thanks again!

@@ -1,12 +1,65 @@
from turtle import color
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not using this anywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was because of auto-import, my bad. I'll remove it.


# draw rectangle with label
y_bottom = y_top
y_top = y_bottom - text_height - 5

if y_top < 0:
print(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea but probably better to use Python's default logging method for things like these.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought using that library would add another dependency, but if you are allowing it, then I'll make the changes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logging is part of Python standard library so it's fine.

return img


def generate_color_map(labels):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like the idea of having different colors for each item in the list. Generally, single color labels are more pleasant to look at.
Not the same thing but here's a study why too many colors/decorations can be distracting

I'd remove this function and remove all instances of it in this library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

return color_map


def draw_detections(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained in the main PR, I do not want this in the library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@@ -22,3 +22,25 @@
img_with_T_labels = bbv.add_multiple_T_labels(img_with_boxes_2, labels, bboxes)

img_with_flags = bbv.draw_multiple_flags_with_labels(img, labels, bboxes)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are not used, please remove them from the file instead of commenting them out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll make the changes.

@shreyasvedpathak
Copy link
Author

shreyasvedpathak commented Aug 31, 2021

I'll make the changes and make the PR, just give me some time (exams😬).
About moving the test files, do you want me to edit the tests created by you as well to maintain consistency of coding style and flow of the code? Or just edit my tests?

@shoumikchow
Copy link
Owner

I'm pretty sure my test files are fine but if you want to edit them, go ahead!

No worries about the delay with the changes. Good luck with your exams!

@shreyasvedpathak
Copy link
Author

@shoumikchow I have made changes as you suggested. I have also checked my test py files using flake8, no errors were reported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants