-
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
Added dai_type, updated encoding flags, and removed reverse_input_channels flag #48
Conversation
…th 'GRAY' as value
… yaml and NN Archive)
… scale=1 on InputConfig
…ixed a bug with multiple inputs and mean/scale values
…hive_from_model methods
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.
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.
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 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
…or checking modified onnx model.
…ue during onnx model update
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.
LGTM, only thing left to update is the requirements.txt to luxonis-ml>=0.5.0
CC: @kozlov721 to re-review
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.
LGTM
Main changes:
encoding.from
andencoding.to
flags to replace thereverse_input_channels
.reverse_input_channels
.dai_type
(its still not 100% compatible with all DAI data types).mean=[0,0,0]
andscale=[1,1,1]