Conversation
PetrNovota
requested review from
kkeroo,
klemen1999 and
tersekmatija
as code owners
|
Is this replacement for #249 and should that one be closed? |
that one can be closed. the #274 is built on top of #249 . This PR is an update for the #274 . Once this PR is ready for merge, the #274 can be then merged immediately. I wanted to make it a separate PR as there are quite a few updates. Hopefuly this is not too confusing |
klemen1999
left a comment
klemen1999
left a comment
There was a problem hiding this comment.
Went through and left some comments, didn't yet test anything.
Since this is adding a bit more complex nodes could we also add some MREs to the PR so we could more easily test these nodes and functionalities (see actual usage)?
depthai_nodes/node/tiles_patcher.py
Outdated
| for nn_msg in nn_msgs | ||
| ] | ||
| merged_detections = merge_messages(remapped_messages) | ||
| if len(remapped_messages) == 0: |
There was a problem hiding this comment.
Could we do this check before remaping on len(nn_msgs)?
| gather_node_data_input = nn_output | ||
|
|
||
| if remap_detections: | ||
| script = self._pipeline.create(dai.node.Script) |
There was a problem hiding this comment.
Why do we need this script node if remap is already performed by CoordinatesMapper node?
There was a problem hiding this comment.
Nice catch, thank you. I missed this. We don't need it.
| def setPadding(self, padding: float) -> None: | ||
| self.detection_cropper.setPadding(padding) | ||
|
|
||
| @property |
There was a problem hiding this comment.
Nitpick on the function order: I would match the ordering as we have it elsewhere so essentially:
- init
- properties
- setters & getters
- build, ran, process functions
- other private functions
There was a problem hiding this comment.
good point. order fixed.
| @@ -0,0 +1,277 @@ | |||
| from typing import Optional, Sequence, Tuple, Union, overload | |||
There was a problem hiding this comment.
General note for all the new nodes:
We should document them as well in the README under depthai_nodes/node/
- add description to README.md - improve arg names
|
@klemen1999 all suggestions integrated, MREs added, just wasn't sure where to put them. let me know if you think another place is better. |
2 participants
Purpose
BaseThreadedHostNodeBaseThreadedHostNode` andBaseHostNodehave the same instance variables,_platform,_pipelineand_logger`.CoordinatesMappernode. Replaces theDetectionsMapper. It is a more general implementation of remapping massage coordinates into different frame of reference.ExtendedNeuralNetworkimplementswith_tilingandwith_detections_filterbuilder methods to split the initialisation more cleanly.Stage2NeuralNetworkso that it follows high level node architecture. It just builds and connects nodes, doesn't have any own logic.remap_messagefunction. Proposing to usefrom_transformationandto_transformationto make it a little bit more clear from where to where the coordinates are mapped.Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
Tested with the updated focused vision app