-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Allow "tiff" and more extensions in DetectionDataset.from_yolo
function
#1636
Allow "tiff" and more extensions in DetectionDataset.from_yolo
function
#1636
Conversation
You never cease to surprise, @patel-zeel; such a thorough analysis! ⭐ Adding this to the back of my review backlog, but I already know it will be a delight. |
Thank you for your kind words, @LinasKo. It's a pleasure to help improve a widely used library. Looking forward to your feedback! |
@LinasKo a small correction. I recently found that Ultralytics uses this function to read |
…zeel/supervision into feat/expand-image-formats
supervision/dataset/formats/yolo.py
Outdated
resolution_wh = (w, h) | ||
if image.mode != "RGB": |
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.
@patel-zeel looks like we can simplify the code here. There is no need for nested ifs.
if image.mode not in ("RGB", "L"):
raise ValueError(
f"Images must be 'RGB' or 'grayscale', but {image_path} mode is '{image.mode}'."
)
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 agree.
supervision/dataset/formats/yolo.py
Outdated
resolution_wh = (w, h) | ||
if image.mode != "RGB": | ||
if image.mode == "L": | ||
image = image.convert("RGB") |
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 seems to me that conversion to RGB is not necessary. The image is not used in the further part of the function.
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.
Right, that'd save us the time wasted in the conversion. I checked the other extreme as well. If we convert all images to RGB by default, that adds a bit of unnecessary overhead. So, this change will improve the speed further. Thank you for pointing it out, @SkalskiP.
supervision/dataset/formats/yolo.py
Outdated
@@ -153,7 +153,7 @@ def load_yolo_annotations( | |||
image_paths = [ | |||
str(path) | |||
for path in list_files_with_extensions( | |||
directory=images_directory_path, extensions=["jpg", "jpeg", "png"] | |||
directory=images_directory_path, extensions=["*"] |
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.
There is a small but important side-effect of this change. If there are other files than images in the directory, we will also try to load them. For example, macOS puts a .DS_Store
file in the directory. @patel-zeel I suggest putting here a list of image extensions that you have tested.
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 agree. On it!
@patel-zeel I apologize for making you wait so long for a review of this PR. I was on a long break at the turn of the year. I left some comments, please address them. |
No worries, @SkalskiP, I hope you had a good time during the break. Thank you for the review! I have addressed the comments and applied the changes. |
@@ -153,7 +153,18 @@ def load_yolo_annotations( | |||
image_paths = [ | |||
str(path) | |||
for path in list_files_with_extensions( | |||
directory=images_directory_path, extensions=["jpg", "jpeg", "png"] | |||
directory=images_directory_path, | |||
extensions=[ |
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.
@patel-zeel small request: don't break file extensions onto separate lines; list them all on one line if possible
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 kept them in a single line @SkalskiP. Pre-commit (specifically ruff-format) forced them on separate lines.
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.
eeeh that's what I thought so
@patel-zeel one more small comment and we should be good to go |
@patel-zeel, thanks a lot! 🙏🏻 approved and merging! |
Description
Addresses #1554 as discussed with @LinasKo.
Highlights
DetectionDataset.from_yolo
.Tested extensions (listed on Ultralytics predict page):
Pillow
could not readpfm
images,cv2
is able to read them. I saw here that even Ultralytics usesPillow
to read images. In that case, I am not sure if Ultralytics can "really" supportpfm
format, as their documentation suggests.ValueError
otherwise.List any dependencies that are required for this change.
Pillow
Type 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?
The changes are tested with this colab notebook.
Test Case
100 images of each extension were generated along with their corresponding dummy labels and dummy
data.yml
. Test run checks if the function works for a particular extension and computes time taken to load the dataset.Time taken (in seconds) to run the test case
-1.11
value indicates thatsupervision
doesn't support that extension yet.Result for
supervision
version0.24.0
, which usescv2
to check the shape of images.Result for this PR, which uses
Pillow
to check the shape and type (RGB or not) of images.Docs