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

Vehicle attributes parser #67

Merged
merged 8 commits into from
Sep 16, 2024
Merged

Vehicle attributes parser #67

merged 8 commits into from
Sep 16, 2024

Conversation

aljazkonec1
Copy link
Contributor

This PR adds a VehicleAttributesParser that is used for vehicle-attributes-classification:72x72 . The model has two classification outputs:

  1. Vehicle type, is one of 4 possible types
  2. Vehicle color, is one of 7 possible colors

The parser returns a message with attributes vehicle_type and vehicle_color each being a Tuple like (class, probability) of the class with the highest probability.

@github-actions github-actions bot added messages Changes affecting ml.messages parsers Changes affecting ml.parsers labels Sep 13, 2024
Copy link

github-actions bot commented Sep 13, 2024

Test Results

143 tests   143 ✅  1s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit b1d42d5.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Sep 13, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
2354 973 41% 0% 🟢

New Files

File Coverage Status
depthai_nodes/ml/messages/composite.py 100% 🟢
depthai_nodes/ml/parsers/vehicle_attributes.py 26% 🟢
TOTAL 63% 🟢

Modified Files

File Coverage Status
depthai_nodes/ml/messages/init.py 100% 🟢
depthai_nodes/ml/messages/creators/init.py 100% 🟢
depthai_nodes/ml/messages/creators/classification.py 94% 🟢
depthai_nodes/ml/messages/creators/misc.py 100% 🟢
depthai_nodes/ml/parsers/init.py 100% 🟢
TOTAL 99% 🟢

updated for commit: b1d42d5 by action🐍

Copy link
Collaborator

@klemen1999 klemen1999 left a comment

Choose a reason for hiding this comment

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

LGTM, I would just like us to think a bit if there is a way to use more general message types instead of custom ones. Because for example create_vehicle_attributes_message (and also VehicleAttributes) are very model specific. These message are nice from the standpoint of user experience (e.g. can get type just by looking at .vehicle_type) but in the longrun this means custom message for every model that is a bit different (e.g. one that predicts brand instead of type).
In general this is a multi-classification model. What if we have a message MultiClassification and its data would be a dict[str, Classification] (mapping between attribute name and it's Classification - scores and class names)? Or perhaps even more composited message type which has dict[str, dai.Buffer] which could be used for this case and also for something like AgeGender?

The drawback of this approach is that these composited types wouldn't have attributes accessable straight on but would have them in a dictionary -> no autocomplete, not that clean,... But I fear that we'll have to create very specific message types for some specific models if we continue down this path.

What are your thoughts on this? Do you have any other possible solutions? (also CC: @kkeroo , @jkbmrz for thoughts)

@jkbmrz
Copy link
Collaborator

jkbmrz commented Sep 13, 2024

I agree with what @klemen1999 wrote and would go with the second option he proposed:

... Or perhaps even more composited message type which has dict[str, dai.Buffer] which could be used for this case and also for something like AgeGender?

I don't think these models will be so common anyways so the drawbacks wouldn't be too annoying and its much nicer to have a clean library than a multitude of custom messages/parsers.

@aljazkonec1
Copy link
Contributor Author

I had the same concerns. I can refactor the code maybe as MultiOutputMessage with data dict[string, [dai.Buffer, float, str, np.ndarray] and then we can make subclasses MultiClassification, AgeGender, ..., Anything that has more than one output? Because AgeGender isnt multi classification but one classification one regression.

@kkeroo
Copy link
Collaborator

kkeroo commented Sep 13, 2024

I dont like dict because of this problem with type hints - it can be really annoying - but we can sacrifice this since we would have example code in the model card. I like your idea @aljazkonec1 with this MultiOutputMessage but I would limit what value types it can handle, e.g. Classifications, Map2D, Keypoints,..., so our "strong" messages? Okay, maybe also float for regression.

I would also generalize your parser, instead of VehicleAttributesParser you can name it MultiClassifcaitonParser and then map the tensor to the names inside the init:

tensor_mapping = {"vehicle_types": 0, "vehicle_color": 1}

and this can then be set via setTensorMapping and defined inside NN archive. These keys can then be used for your dictionary multiclass msg.

@aljazkonec1
Copy link
Contributor Author

Okay I will change accordingly to multiclass and also update nn_archive to incorperate multiple heads.

@klemen1999
Copy link
Collaborator

klemen1999 commented Sep 13, 2024

but I would limit what value types it can handle, e.g. Classifications, Map2D, Keypoints,..., so our "strong" messages? Okay, maybe also float for regression.

Agree, since we sacrifice in type hints we should then profit in the lesser amount of custom messages needed (try to keep only strong ones). This MultiOutputMessage could be then built in the parser itself: if we take AgeGender for example parser creates a Classification message for gender and float for age, we package this into MultiOutputMessage and return it (similarly for vehicle model we can have 2 Classification messages packaged together). Does this make sense?

@aljazkonec1
Copy link
Contributor Author

Changed to MultiClassification Parser.

Also made a Miscellaneous message that only saves a dictionary. The dictionary can only have values dai.Buffer, float or np.ndarray. To get the dictionary you can call .getData() method thats overwritten from the dai.Buffer class. Therefore at least some autofill is present when using the parser/message.

depthai_nodes/ml/messages/base_message.py Outdated Show resolved Hide resolved
depthai_nodes/ml/messages/creators/misc.py Outdated Show resolved Hide resolved
depthai_nodes/ml/messages/misc.py Outdated Show resolved Hide resolved
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.

create_age_gender_message and AgeGender will also need to be changed accordingly but we can do that in a separate PR. LGTM otherwise.

Copy link
Collaborator

@klemen1999 klemen1999 left a comment

Choose a reason for hiding this comment

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

LGTM

depthai_nodes/ml/parsers/vehicle_attributes.py Outdated Show resolved Hide resolved
@aljazkonec1 aljazkonec1 merged commit ceac1c8 into dev Sep 16, 2024
10 of 11 checks passed
@aljazkonec1 aljazkonec1 deleted the vehicle-attributes-parser branch September 16, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
messages Changes affecting ml.messages parsers Changes affecting ml.parsers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants