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

Refactor Code #68

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Refactor Code #68

merged 3 commits into from
Sep 17, 2024

Conversation

aljazkonec1
Copy link
Contributor

This PR Refactors parts of the code. Some examples include:

  • moving all classification messages into a single file,
  • removing all mentions of vehicles from multiclassification,
  • refactoring create_age_gender_message to return CompositeMessage instead of AgeGenderMessage.

@github-actions github-actions bot added the parsers Changes affecting ml.parsers label Sep 17, 2024
Copy link

github-actions bot commented Sep 17, 2024

Test Results

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

Results for commit 13d7d4a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Sep 17, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
2319 940 41% 0% 🟢

New Files

No new covered files...

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 98% 🟢
depthai_nodes/ml/messages/creators/misc.py 100% 🟢
depthai_nodes/ml/parsers/init.py 100% 🟢
depthai_nodes/ml/parsers/classification.py 21% 🟢
depthai_nodes/ml/parsers/ppdet.py 31% 🟢
TOTAL 79% 🟢

updated for commit: 13d7d4a by action🐍

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.

LGTM, did you delete AgeGender msg from messages? Did you test that all parsers that are affected work and that examples work?

Not sure if we want to have to parsers in the same file or not (Classification and MultiClassification parsers)? CC @klemen1999 @jkbmrz

@github-actions github-actions bot added the messages Changes affecting ml.messages label Sep 17, 2024
@aljazkonec1
Copy link
Contributor Author

Thanks for noticing, yes I forgot to remove AgeGender message.

All message creators are working, I also tested AgeGender, PaddleOCR, VehicleAttributes parsers. In the example none of these parsers are used so there should be no error.

@kkeroo
Copy link
Collaborator

kkeroo commented Sep 17, 2024

AgeGender model is used in examples. Can you adjust the code so it works?

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

@aljazkonec1 aljazkonec1 merged commit a6d34c1 into dev Sep 17, 2024
10 of 11 checks passed
@aljazkonec1 aljazkonec1 deleted the update-parser-locations branch September 17, 2024 09:10
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.

3 participants