Skip to content

Conversation

@dominik737
Copy link
Contributor

Purpose

Addition of GatherData node. The node can by default gather number of NNData specified by the length of dai.ImgDetections.detections with the same (or in tolerance) timestamp. The node can be used a n-stage sync.

Specification

The GatherData node and the related tests are added. The getAll method was added to the mock Queue. The mock Queue.send was changed such that the Infinite queues work as FIFO not LIFO. This makes the items in the Queue being cycled, instead of the last inserted being always returned.

Dependencies & Potential Impact

The change of the mock Queue to FIFO could change behavior of dependent tests.

Deployment Plan

None / not applicable

Testing & Validation

The tests for GatherData were made and are passing.

Copilot AI review requested due to automatic review settings April 11, 2025 18:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

depthai_nodes/node/gather_data.py:139

  • Consider using '<=' rather than '<' in the tolerance check so that timestamps exactly on the threshold are considered matching, if that matches the intended behavior.
return difference < (1 / self._camera_fps / self.FPS_TOLERANCE_DIVISOR)

depthai_nodes/node/gather_data.py:195

  • Using floating point values as dictionary keys for timestamps may lead to precision errors when matching; consider rounding or normalizing the timestamp value to a fixed precision before storing or comparing.
def _get_total_seconds_ts(self, buffer_like: dai.Buffer) -> float:

@github-actions github-actions bot added the enhancement New feature or request label Apr 11, 2025
@dominik737 dominik737 mentioned this pull request Apr 11, 2025
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 96.40719% with 6 lines in your changes missing coverage. Please review.

Project coverage is 53.31%. Comparing base (5c6073a) to head (270a49e).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
depthai_nodes/node/gather_data.py 96.40% 5 Missing ⚠️
depthai_nodes/message/gathered_data.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   51.80%   53.31%   +1.50%     
==========================================
  Files          81       83       +2     
  Lines        4799     4967     +168     
==========================================
+ Hits         2486     2648     +162     
- Misses       2313     2319       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@klemen1999 klemen1999 requested a review from aljazkonec1 April 14, 2025 07:47
Copy link
Contributor

@jkbmrz jkbmrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, just adding two small comments

Copy link
Contributor

@aljazkonec1 aljazkonec1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the logic is the same as in previous PR and is good. Left some small comments on the docstrings.

@dominik737 dominik737 requested a review from kkeroo April 15, 2025 07:58
Copy link
Collaborator

@kkeroo kkeroo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dominik737 dominik737 requested a review from aljazkonec1 April 15, 2025 08:26
Copy link
Contributor

@aljazkonec1 aljazkonec1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just the last docstring update

@dominik737 dominik737 merged commit 9f9e4ed into main Apr 15, 2025
12 checks passed
@dominik737 dominik737 deleted the feat/gather-data branch April 15, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants