-
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
Expose parsers when building a pipeline through ParsingNeuralNetwork #149
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
=======================================
Coverage 33.29% 33.30%
=======================================
Files 69 69
Lines 3835 3840 +5
=======================================
+ Hits 1277 1279 +2
- Misses 2558 2561 +3 ☔ View full report in Codecov by Sentry. |
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 LGTM
@@ -127,6 +127,13 @@ def setBackend(self, setBackend: str) -> None: | |||
"""Sets the backend of the NeuralNetwork node.""" | |||
self._nn.setBackend(setBackend) | |||
|
|||
def getParser(self, parserID: int = 0) -> BaseParser: |
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.
What are your thoughts on having default value here? The other option is that you have to explicitly speciffy the parserID but I agree that in 90% of the cases users are dealing with 1parser NNs so this would be then the desired behaviour. CC: @jkbmrz @kkeroo @aljazkonec1
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.
We could also set the default value to None
and if no value is provided:
- return the parser at zero index if there is only one parser available,
- error out if there is multiple parsers available to avoid mistakes.
BTW, is parserID
a good naming? I think parser_index
would be more appropriate.
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.
Having an int value set to None would work but could cause issues down the line when porting to depthai. The function name getParser
implies that only one is returned and I would just update the docstring such that it explicitly states which one is returned ( i.e. something like: The first parser is returned by default if multiple parsers available).
Regarding the naming, I would switch to snake case (to be consistent with other variables/attributes). index
does make slightly more sense.
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.
I would leave the default option, so calling getParser()
returns the first one. I'm fine with both ways of doing it (Petros' way and Jakob's way).
parserID = 0