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

Added dai_type to NNArchive #200

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

klemen1999
Copy link
Contributor

@klemen1999 klemen1999 commented Nov 5, 2024

Main changes:

  • config_version is a string in format "x.y" where x and y are integers
  • YOLO subtype is a string and not an Enum
  • Changed description of reverse_input_channels and interleaved_to_planar to match their real meaning
    • Added deprecation warning on these two flags
  • Added dai_type to the archive which is what DAI will read to automatically setup the pipeline

@klemen1999 klemen1999 requested a review from a team as a code owner November 5, 2024 16:42
@klemen1999 klemen1999 requested review from kozlov721, tersekmatija and conorsim and removed request for a team November 5, 2024 16:42
@github-actions github-actions bot added enhancement New feature or request NN Archive Changes affecting luxonis_ml.nn_archive subpackage labels Nov 5, 2024
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.52%. Comparing base (6bcbb11) to head (460f83f).
Report is 13 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
+ Coverage   92.33%   92.52%   +0.19%     
==========================================
  Files          79       79              
  Lines        4161     4227      +66     
==========================================
+ Hits         3842     3911      +69     
+ Misses        319      316       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@kozlov721 kozlov721 left a comment

Choose a reason for hiding this comment

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

Left a small comment, otherwise LGTM

deprecated="Deprecated, use `dai_type` instead.",
description="If True input to the model is interleaved (NHWC) else planar (NCHW).",
)
dai_type: str = Field(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably validate the value

Copy link

Choose a reason for hiding this comment

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

+1 on validating this str value. Maybe have a list of all the possible data types from DAI and ensure the given value exists there?

Copy link

@ptoupas ptoupas left a comment

Choose a reason for hiding this comment

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

Just left a couple of comments, otherwise LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NN Archive Changes affecting luxonis_ml.nn_archive subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants