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, updated encoding flags, and removed reverse_input_channels flag #48

Merged
merged 27 commits into from
Nov 11, 2024

Conversation

ptoupas
Copy link
Contributor

@ptoupas ptoupas commented Nov 8, 2024

Main changes:

  • Updated the encoding.from and encoding.to flags to replace the reverse_input_channels.
  • Removed reverse_input_channels.
  • Added dai_type (its still not 100% compatible with all DAI data types).
  • Fixed a bug with mean and scale values on RVC2 conversion with multiple inputs.
  • Removed the extra nodes added to the onnx model when mean=[0,0,0] and scale=[1,1,1]
  • Added extra unit tests for the encoding

@ptoupas ptoupas added enhancement New feature or request fix Fixes a bug tests Adding or changing tests NN Archive Changes affecting NN Archive export labels Nov 8, 2024
@ptoupas ptoupas self-assigned this Nov 8, 2024
@ptoupas ptoupas requested a review from a team as a code owner November 8, 2024 08:37
@ptoupas ptoupas requested review from kozlov721, klemen1999, tersekmatija and conorsim and removed request for a team November 8, 2024 08:37
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.

In general LGTM, just left a comment regarding removing the reverse_input_channels. I'd propose keeping it as a property of InputConfig, somehting like:

@property
def reverse_input_channels(self) -> bool:
    return self.encoding.to != self.encoding.from_

That way we don't need to change the rest of the code where inp.reverse_input_channels is used. And it also looks much more readable than doing inp.encoding.to != inp.encoding.from_ everywhere.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
modelconverter/utils/config.py Show resolved Hide resolved
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.

Generally looks good. I left some comments but mainly what we are missing is rigorous testing. By that I mean:

  • test of the output NNArchive for different input configs, intermediate NNArchives and/or CLI params
  • tests of intermediate onnx (for RVC4) to check which blocks are added and if values inside them are correct (mean and scale properly reversed if need be)
  • test that flags for RVC2/3 and HAILO conversion are properly set up depending on the input config
  • any other tests that will help catch any current or future bugs

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
modelconverter/packages/rvc2/exporter.py Show resolved Hide resolved
modelconverter/packages/hailo/exporter.py Outdated Show resolved Hide resolved
modelconverter/utils/onnx_tools.py Show resolved Hide resolved
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, only thing left to update is the requirements.txt to luxonis-ml>=0.5.0
CC: @kozlov721 to re-review

@ptoupas ptoupas requested a review from kozlov721 November 11, 2024 17:17
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.

LGTM

@ptoupas ptoupas merged commit 9e1a163 into main Nov 11, 2024
5 checks passed
@ptoupas ptoupas deleted the feat/nnarchive-flags-update branch November 11, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix Fixes a bug NN Archive Changes affecting NN Archive export tests Adding or changing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants