Skip to content

Feature/ultralytics all models for Classification #281

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

Merged
merged 15 commits into from
Aug 15, 2023

Conversation

mayankagarwals
Copy link
Contributor

@mayankagarwals mayankagarwals commented Aug 11, 2023

@SkalskiP
Copy link
Collaborator

Hi, @mayankagarwals 👋🏻! Could you create a small Google Colab to test that change?

@@ -40,6 +42,9 @@ def __post_init__(self) -> None:
_validate_confidence(self.confidence, n)

@classmethod
@deprecated(
"This method is deprecated and removed in 0.15.0 release. Use sv.Classifications.from_ultralytics() instead."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make that 0.16.0. We should give people at least 2 full release cycles to migrate. That change will be released with 0.14.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

>>> import supervision as sv

>>> image = cv2.imread(SOURCE_IMAGE_PATH)
>>> model = YOLO('yolov8s.pt')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's update the example to use only detection and segmentation models here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't quite get you. yolov8s.pt is a detection model itself. We can add a classification model here if that's what you mean

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is my point exactly. We are documenting sv.Classifications.from_ultralytics but we are showing examples of usage with detection models. We should show classification model examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Although I can see only yolo models for classification on ultralytics https://docs.ultralytics.com/tasks/classify/

@SkalskiP
Copy link
Collaborator

Hi @artyaltanzaya 👋🏻 I did an initial round of reviews on this PR. Please take it from here 🙏🏻 and cooperate with @mayankagarwals to make sure we will be able to release that change with supervision-0.14.0. Take a look here for more context.

@mayankagarwals
Copy link
Contributor Author

Hi @SkalskiP

Have added a testing colab in the description.
Have updated the version in docstring for both classification and detection . Hope that's fine

@SkalskiP SkalskiP added the version 0.14.0 Feature to be added in `0.14.0` release label Aug 11, 2023
@SkalskiP SkalskiP added this to the version: 0.14.0 milestone Aug 11, 2023
@SkalskiP
Copy link
Collaborator

@mayankagarwals 🙏🏻 please also make sure to resolve supervision/detection/core.py conflicts.

Copy link

@artyaltanzaya artyaltanzaya left a comment

Choose a reason for hiding this comment

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

Thank you for the updates 👍. Added two comments! One is resolved, thanks for the quick action.

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2023

CLA assistant check
All committers have signed the CLA.

@mayankagarwals
Copy link
Contributor Author

@onuralpszr Couple things with the pre-commit-ci

  1. Can you help me with getting the cla part resolved

  2. I guess this hook is more restricting than supervision's existing flake8 tests. Just FYI.

@onuralpszr
Copy link
Collaborator

onuralpszr commented Aug 15, 2023

@onuralpszr Couple things with the pre-commit-ci

  1. Can you help me with getting the cla part resolved

  2. I guess this hook is more restricting than supervision's existing flake8 tests. Just FYI.

For CLA I need to ask because I did not added. For flake it I only saw long line error so it is normal. :) Also you can squash those changes for make it look clean :)

@onuralpszr
Copy link
Collaborator

@onuralpszr
Copy link
Collaborator

@mayankagarwals CLA part has been solved, thanks to @capjamesg.

@mayankagarwals
Copy link
Contributor Author

For flake it I only saw long line error so it is normal. :) Also you can squash those changes for make it look clean :)

Yes yes, just FYI that there is a minor behavior change. If it's intentional, no issues

CLA part has been solved, thanks to @capjamesg.

Amazing, thanks! @artyaltanzaya All issues are addressed :)

@SkalskiP
Copy link
Collaborator

@mayankagarwals I found a small problem during the final tests. You can take a look here. Committed a fix into your branch. It looks like we are ready to merge.

@SkalskiP
Copy link
Collaborator

@capjamesg thanks for solving issues with bot users 🙏🏻

@SkalskiP SkalskiP merged commit bfe6501 into roboflow:develop Aug 15, 2023
@mayankagarwals
Copy link
Contributor Author

@mayankagarwals I found a small problem during the final tests. You can take a look here. Committed a fix into your branch. It looks like we are ready to merge.

@SkalskiP Thanks for the fix

That's quite a crucial fix. Apologies for the lapse on my part, it's my first commit to the classification part of codebase. Looks like I got the semantics of the Classifications feature wrong. For some reason I thought it was class of bounding boxes classified. That's my bad.

Your fix does solve that. My only worry is probs.data isn't a documented field: https://docs.ultralytics.com/reference/engine/results/#ultralytics.engine.results.Probs which could mean not long term supported. But I guess it's okay for now as their documented functions don't cover all our use cases.

Again, thanks for the support!

@SkalskiP
Copy link
Collaborator

That's quite a crucial fix. Apologies for the lapse on my part, it's my first commit to the classification part of codebase. Looks like I got the semantics of the Classifications feature wrong. For some reason I thought it was class of bounding boxes classified. That's my bad.

No worries! That's why we have multiple people looking at the code and testing. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version 0.14.0 Feature to be added in `0.14.0` release
Projects
Status: Current Release: Done
Development

Successfully merging this pull request may close these issues.

5 participants