-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adds support for unicode labels with new RichLabelAnnotator
class
#1116
Adds support for unicode labels with new RichLabelAnnotator
class
#1116
Conversation
Hi @onuralpszr @SkalskiP, just wanted to nudge you on this PR when you have a moment. Thanks a lot! |
…r fixes Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
e978dd7
to
d983177
Compare
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.
LGTM, I made documentation additions, I also wanted to test with my new collab to see work as intended. Another collaboration works but I also noticed init was missing in the code so I made sure it works as well.
I am also sorry for late review. |
@jeslinpjames merging in. Thank you for your work 🎉 🚀 |
Thanks for the merge and your review! No worries about the delay. |
Hi @onuralpszr, @jeslinpjames, Apologies for the delay on our part. Despite it being merged, I wanted to look at the code now. |
@LinasKo do you want my version collab ? (for testing purpose) |
@onuralpszr, That would be good to have :) |
@onuralpszr, I'm getting a "you need access" message |
oh let me fix it. |
@LinasKo link updated |
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 @onuralpszr, @jeslinpjames,
Same issue as before - I had left the comments on "pending", apparently.
Would any of you have some time to make a few additional changes?
This can be in a separate PR.
self, | ||
scene: ImageType, | ||
detections: Detections, | ||
labels: List[str] = None, |
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.
This should be Optional[...]
And yes - I'm aware LabelAnnotator is similarly incorrect.
`ImageType` is a flexible type, accepting either `numpy.ndarray` | ||
or `PIL.Image.Image`. | ||
detections (Detections): Object detections to annotate. | ||
labels (List[str]): Optional. Custom labels for each detection. |
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.
This should be (Optional[List[str]])
f"detections are not aligned or if an incorrect number of labels has " | ||
f"been provided. Please ensure that the labels array has the same " | ||
f"length as the Detections object." | ||
) |
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.
It's nice when libraries tell what exactly is wrong :)
Still, we can make it more concise. We can remove this:
This discrepancy can occur if the labels and "
f"detections are not aligned or if an incorrect number of labels has "
f"been provided. Please ensure that the labels array has the same "
f"length as the Detections object.
import supervision as sv | ||
|
||
image = ... | ||
detections = sv.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.
Same here - we need a specific model that provides class_name
I see LabelAnnotator is the same. I'll update both later on.
self.font = ImageFont.load_default(size=font_size) | ||
else: | ||
self.font = ImageFont.load_default(size=font_size) | ||
|
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 see why you skipped the @convert_for_annotation_method
annotator - you need PIL images here.
What it does is allows user to use either PIL and np.ndarray images, ensuring that only np.ndarray images reach the annotator.
- Let's leave a comment here to prevent future confusion.
# Needs PIL images. Should not use standard annotator decorator
. Maybe there's a more concise way or better phrasing - that'd be even better. - Since it's the only annotator needing PIL, let's implement the conversions in
annotate
, rather than in a decorator. Before converting to PIL, store the datatype in a variable. Before returning, if a numpy array was originally passed as the argument, convert the result to an array.
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.
1% better wording: Requires PIL images. Should not use the annotator input decorator
fill=self.text_color.as_rgb(), | ||
) | ||
|
||
return scene |
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.
This is now always a PIL image. Let's check if the user passed a np.ndarray
originally and make sure we return the image in the same format.
Let me read those and I can provide PR for changes If you like and add deprecated warning |
@onuralpszr Which deprecation warning are we talking about? If you meant deprecating
|
self, | ||
color: Union[Color, ColorPalette] = ColorPalette.DEFAULT, | ||
text_color: Color = Color.WHITE, | ||
font_path: str = None, |
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.
Should be Optional[str]
I meant you wanna change params, we can put deprecated warning and fix it so we can have both solutions (keeping old for a while and having proper fix for next release) Edit : Because deprecated also keep old parameter and still accept it. |
Alright, I see your point. This time, the merge wasn't part of any package releases, so there shouldn't be anything to deprecate, but that's a good idea to keep in mind for future updates. |
Description
Issue #990
New class
RichLabelAnnotator
added andresolve_text_background_xyxy()
function moved fromLabelAnnotatorClass
to utils.py in supervision\annotatorsList any dependencies that are required for this change.
ImageDraw
,ImageFont
,Image
from PILType of change
Please delete options that are not relevant.
How has this change been tested, please provide a testcase or example of how you tested the change?
Here is the Colab link with the new
RichLabelAnnotator
Class.Any specific deployment considerations
For example, documentation changes, usability, usage/costs, secrets, etc.
Docs