-
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
Host nodes: DepthMerger. #163
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
==========================================
+ Coverage 36.05% 37.78% +1.72%
==========================================
Files 72 75 +3
Lines 4024 4137 +113
==========================================
+ Hits 1451 1563 +112
- Misses 2573 2574 +1 ☔ 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.
Looks good. I would have a nodes
folder along (same level as) the ml
folder. Since many of the nodes are actually not related to ml. Then if there are too many nodes, we can group then in the nodes
folder into subfolders.
@klemen1999 Also I just remembered we were talking about tests. Should they be implemented now in those PRs that add nodes? |
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
Agree on moving nodes into separate folder (same level as ml) and not nesting them into subfolders at least for now, we can do that later on if needed.
@klemen1999 Also I just remembered we were talking about tests. Should they be implemented now in those PRs that add nodes?
Good catch, we should make tests for every new node as potentially many different experiments will rely on their specifics. We can sync offline what/how to test.
Okay, I moved the added DepthMerger node to the |
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. Just adding a small stylistic suggestion.
Also, I agree on having the nodes
folder at the same level as ml
. I'm wondering, though, if we should also move there the contents of thedepthai_nodes/ml/helpers
folder?
I added the tests for |
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, thanks!
This PR adds first host node called DepthMerger. The node merges normal
ImgDetections
orImgDetectionsExtended
with depth information intoSpatialImgDetections
. The node is taken from experiments host-nodes folder and slightly adjusted so it works withImgDetectionsExtended
. We also introduce parametershrinking_factor
that is responsible for optionally shrinking the ROI for calculating depth values. This is useful because sometimes bounding-box also includes background and so the mean depth value is affected. E.g. In bouding-box of a human the shrinking factor will shrink the ROI into the human torso.I am not sure about the folder hierarchy. I was thinking of placing it in the
helpers
folder under specific subfolders. Maybe there is something better?The proposed node works with rewritten experiments: Human-Machine safety and social distancing.