-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: master
Are you sure you want to change the base?
Labelling issue and Bounding box drawing issue. #8
Conversation
I added this feature request (#9) (made by me lol). Issue: A single function to draw detections with labelling style as a parameter. Description: Misc:
|
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.
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!
bbox_visualizer/bbox_visualizer.py
Outdated
@@ -1,12 +1,65 @@ | |||
from turtle import color |
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.
You are not using this anywhere
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.
I guess it was because of auto-import, my bad. I'll remove it.
bbox_visualizer/bbox_visualizer.py
Outdated
|
||
# draw rectangle with label | ||
y_bottom = y_top | ||
y_top = y_bottom - text_height - 5 | ||
|
||
if y_top < 0: | ||
print( |
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.
Good idea but probably better to use Python's default logging method for things like these.
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.
I thought using that library would add another dependency, but if you are allowing it, then I'll make the changes.
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.
logging is part of Python standard library so it's fine.
bbox_visualizer/bbox_visualizer.py
Outdated
return img | ||
|
||
|
||
def generate_color_map(labels): |
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.
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.
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.
Will do.
bbox_visualizer/bbox_visualizer.py
Outdated
return color_map | ||
|
||
|
||
def draw_detections( |
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.
As explained in the main PR, I do not want this in the library.
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.
Will do.
demo/multiple_objects.py
Outdated
@@ -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) | |||
|
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.
If these are not used, please remove them from the file instead of commenting them out.
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.
Okay, I'll make the changes.
I'll make the changes and make the PR, just give me some time (exams😬). |
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! |
ba605ee
to
7eb4c0a
Compare
@shoumikchow I have made changes as you suggested. I have also checked my test py files using flake8, no errors were reported. |
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 functioncheck_and_modify_bbox()
, it can be found in the filebbox_visualizer.py
line number 3. I also added amargin
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.