Bugfix: increase performances of filter function

[Grufoony]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.22%. Comparing base (c10ef00) to head (0ce9542).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/dsf/mdt/TrajectoryCollection.cpp	60.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   83.20%   83.22%   +0.02%     
==========================================
  Files          52       52              
  Lines        5233     5240       +7     
  Branches      596      599       +3     
==========================================
+ Hits         4354     4361       +7     
  Misses        867      867              
  Partials       12       12              

Flag	Coverage Δ
unittests	83.22% <60.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR attempts to improve the performance of the filter function in TrajectoryCollection by optimizing lambda captures, but introduces critical bugs in the loop iteration logic that break the filtering functionality.

Key Changes:

• Changed lambda captures from by-value to by-reference to reduce copying overhead
• Modified loop structure to use pre-increment in the condition check
• Split timestamp validation into separate cases for strictly decreasing (error) vs equal (warning)

Comments suppressed due to low confidence (3)

src/dsf/mdt/TrajectoryCollection.cpp:165

• The loop increment bug (where every other point is skipped) is not caught by existing tests. The tests only verify that the filter doesn't crash, not that it processes all points correctly. Consider adding a test that:

1. Creates a trajectory with an even number of points where consecutive pairs alternate between passing and failing the max speed check
2. Verifies that all expected violations are detected, not just half of them

This would catch the bug where ++i in the loop condition causes the loop to skip processing every other point.

          for (std::size_t i = 0; i < nPoints;) {
            auto const& currentCluster = points[i];
            if (!check_min_duration(currentCluster)) {
              to_split.insert(uid);
              return;
            }
            if (++i < nPoints) {
              auto const& nextCluster = points[i];
              if (!check_max_speed(nextCluster, currentCluster)) {
                to_split.insert(uid);
                return;
              }
            }

src/dsf/mdt/TrajectoryCollection.cpp:165

• The loop increment logic is incorrect. The variable i is incremented in the loop condition check (++i < nPoints), but the loop body also processes points[i] after the increment. This causes the loop to skip every other point.

When the loop executes:

1. Iteration 1: i=0, processes points[0] as currentCluster, increments to i=1, processes points[1] as nextCluster
2. Iteration 2: Loop condition increments i to 2, skips points[1] processing, processes points[2] as currentCluster, etc.

The original code with i > 0 correctly checked the previous cluster without skipping points. If the intent is to check consecutive pairs, the increment should happen at the end of the loop body, not in the condition.

          for (std::size_t i = 0; i < nPoints;) {
            auto const& currentCluster = points[i];
            if (!check_min_duration(currentCluster)) {
              to_split.insert(uid);
              return;
            }
            if (++i < nPoints) {
              auto const& nextCluster = points[i];
              if (!check_max_speed(nextCluster, currentCluster)) {
                to_split.insert(uid);
                return;
              }
            }

src/dsf/mdt/TrajectoryCollection.cpp:212

• The same loop increment bug exists here. The loop increments i in the condition check (++i < nPoints), but then continues to process points[i] as nextCluster. This means:

1. The loop skips every other point for processing
2. After adding currentCluster at index 0, incrementing to 1, and checking speed between points[1] and points[0], the next iteration jumps to index 2, never properly processing point 1 as a currentCluster

This breaks the trajectory splitting logic and causes incorrect behavior.

      for (std::size_t i = 0; i < nPoints;) {
        auto const& currentCluster = points[i];
        newTrajectory.addCluster(currentCluster);

        bool bShouldSplit = false;

        if (++i < nPoints) {
          auto const& nextCluster = points[i];
          bShouldSplit = !check_max_speed(nextCluster, currentCluster);
        }
        if (!bShouldSplit) {
          bShouldSplit = !check_min_duration(currentCluster);
        }
        // If constraint violated (max speed or min duration) - finalize current trajectory and start a new one
        if (bShouldSplit && !newTrajectory.empty()) {
          if (newTrajectory.size() >= min_points_per_trajectory) {
            trajectories.emplace_back(std::move(newTrajectory));
          }
          newTrajectory = Trajectory();
          newTrajectory.addCluster(currentCluster);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Changing the log level from warn to debug for equal timestamps reduces visibility of data quality issues. Non-increasing timestamps could indicate data quality problems that should be surfaced to users, not hidden in debug logs. Consider keeping this as a warning or making the log level configurable.

Suggested change

	spdlog::debug(
	spdlog::warn(

[Copilot]

Throwing an exception for non-increasing timestamps in a parallel loop can cause resource leaks or incomplete cleanup. The tbb::parallel_for_each may have multiple threads processing trajectories, and an exception from one thread could leave the concurrent sets (to_remove, to_split) in an inconsistent state.

Consider either:

1. Collecting problematic trajectory IDs to remove/handle after the parallel section
2. Using spdlog::error() and marking the trajectory for removal instead of throwing
3. Documenting that input data must be pre-validated for timestamp ordering