-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
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.
Generally looks okay! Adding mostly some comments on the style.
import depthai as dai | ||
|
||
|
||
class ClassificationMessage(dai.Buffer): |
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.
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
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.
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
?
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.
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
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.
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?
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 changed the latest version such that the ClassificationMessage has classes
and scores
, both being just a simple list as to not introduce unecessary libraries.
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.
@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.
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.
Looks good. Some minor improvements are suggested for parser to be consistent with the repo.
import depthai as dai | ||
|
||
|
||
class ClassificationMessage(dai.Buffer): |
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.
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)
@aljazkonec1 You should also add this line for adding timestamps to the created msg (after the |
Added ClassificationParser and ClassificationMessage classes that are compatible with DepthAI 3 and follow a common naming convention.