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

Classification parser #15

Merged
merged 9 commits into from
Aug 21, 2024
Merged

Classification parser #15

merged 9 commits into from
Aug 21, 2024

Conversation

aljazkonec1
Copy link
Contributor

Added ClassificationParser and ClassificationMessage classes that are compatible with DepthAI 3 and follow a common naming convention.

@aljazkonec1 aljazkonec1 requested review from kkeroo and jkbmrz and removed request for kkeroo July 29, 2024 08:55
Copy link
Collaborator

@jkbmrz jkbmrz left a comment

Choose a reason for hiding this comment

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

Generally looks okay! Adding mostly some comments on the style.

.gitignore Show resolved Hide resolved
import depthai as dai


class ClassificationMessage(dai.Buffer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to Classifications to follow the structure of the repo? OR Should we maybe split this into Classification (one class_name, one score) and Classifications (list of Classification objects) classes? This way we keep the structure similar as in ml/messages/img_detections.py. CC @kkeroo on this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree. I would split it into a list of Classification objects. Additionally, I would change the representation from [classname, score] into classification.class = classname and classification.score = score?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't this introduce unnecesary abstraction? To then get a list of all class names and their probabilities from the parser they would have to do:

queue = class_parser.out.createOutputQueue()
message = queue.get().list_of_classification_objects
classes = []
probabilities = []
for classification in message:
    classes.append(classification.class)
    probabilities.append(classification.score)

I would rather do classification.classes be sorted list of classes and classification.scores be the corresponding list of probabilities. Then the user can get both by simply:
message = queue.get()
classes = message.classes
probability_scores = message.scores

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I see your point. In your snippet you dont need to store classes and probabilities to two lists. You just have classifications each having class and score. If you want to access one classification's class and score you just go with classification.score and classification.classname. Also, potentially classification can be reused in other msgs.
However, I am not 100% sure whats best. Thoughts @tersekmatija @jkbmrz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the latest version such that the ClassificationMessage has classes and scores , both being just a simple list as to not introduce unecessary libraries.

Copy link
Collaborator

@jkbmrz jkbmrz Aug 6, 2024

Choose a reason for hiding this comment

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

@aljazkonec1 I see your point. We could overcome this issue by writing getter() methods for scores and classes. However, I guess this would get us to the same functionality as currently so we can leave it as it is for now and we separate it into two objects if we later figure out that Classification message can be re-used somewhere else as pointed out by @kkeroo.

depthai_nodes/ml/messages/classification.py Outdated Show resolved Hide resolved
depthai_nodes/ml/messages/classification.py Outdated Show resolved Hide resolved
depthai_nodes/ml/parsers/classification_parser.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkeroo kkeroo left a comment

Choose a reason for hiding this comment

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

Looks good. Some minor improvements are suggested for parser to be consistent with the repo.

import depthai as dai


class ClassificationMessage(dai.Buffer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree. I would split it into a list of Classification objects. Additionally, I would change the representation from [classname, score] into classification.class = classname and classification.score = score?

…ave two attributes: scores and classes, instead of a list of shape (n_classes, 2)
@kkeroo
Copy link
Collaborator

kkeroo commented Aug 19, 2024

@aljazkonec1 You should also add this line for adding timestamps to the created msg (after the msg = ...): msg.setTimestamp(output.getTimestamp())

@aljazkonec1 aljazkonec1 merged commit 136de99 into main Aug 21, 2024
1 check passed
@aljazkonec1 aljazkonec1 deleted the classification_parser branch August 21, 2024 08:05
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.

5 participants