Skip to content

Bugfix: increase performances of filter function #372

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.

Sign up for GitHub

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

Merged
Grufoony merged 2 commits into from
Nov 27, 2025
Merged

Bugfix: increase performances of filter function #372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c48f4cc
Bugfix: increase performances of `filter` function
Grufoony Nov 27, 2025
0ce9542
Bump version
Grufoony Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/dsf/dsf.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

static constexpr uint8_t DSF_VERSION_MAJOR = 4;
static constexpr uint8_t DSF_VERSION_MINOR = 4;
static constexpr uint8_t DSF_VERSION_PATCH = 5;
static constexpr uint8_t DSF_VERSION_PATCH = 6;

static auto const DSF_VERSION =
std::format("{}.{}.{}", DSF_VERSION_MAJOR, DSF_VERSION_MINOR, DSF_VERSION_PATCH);
Expand Down
60 changes: 35 additions & 25 deletions src/dsf/mdt/TrajectoryCollection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,21 @@
tbb::concurrent_set<Id> to_split;

// Returns true if speed between two points is below max_speed_kph
auto check_max_speed = [](PointsCluster const& currentCluster,
PointsCluster const& previousCluster,
double const max_speed_kph) {
auto check_max_speed = [&max_speed_kph](PointsCluster const& currentCluster,

Check notice

Code scanning / Cppcheck (reported by Codacy)

MISRA 12.3 rule Note

MISRA 12.3 rule
PointsCluster const& previousCluster) {

Check notice

Code scanning / Cppcheck (reported by Codacy)

MISRA 13.1 rule Note

MISRA 13.1 rule
auto const distance_km = dsf::geometry::haversine_km(currentCluster.centroid(),
previousCluster.centroid());
auto const current_time =
(currentCluster.lastTimestamp() + currentCluster.firstTimestamp()) * 0.5;
auto const previous_time =
(previousCluster.lastTimestamp() + previousCluster.firstTimestamp()) * 0.5;
if (current_time <= previous_time) {
spdlog::warn(
if (current_time < previous_time) {
// Should never happen if data is clean
throw std::runtime_error(
"Timestamps are not in increasing order within the trajectory.");
Comment on lines +102 to +105
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

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

Copilot uses AI. Check for mistakes.
}
if (current_time == previous_time) {
spdlog::debug(
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

[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 uses AI. Check for mistakes.
"Non-increasing timestamps detected. Skipping speed check for these points.");
return true;
}
Expand All @@ -110,18 +114,21 @@
return speed_kph <= max_speed_kph;
};
// Returns true if cluster duration is below min_duration_min
auto check_min_duration = [](dsf::mdt::PointsCluster const& cluster,
std::time_t const min_duration_min) {
return cluster.duration() < min_duration_min * SECONDS_IN_MINUTE;
};
auto check_min_duration =
[&min_duration_min](dsf::mdt::PointsCluster const& cluster) {
if (!min_duration_min.has_value()) {
return true;

Check notice

Code scanning / Cppcheck (reported by Codacy)

MISRA 15.5 rule Note

MISRA 15.5 rule
}
return cluster.duration() < min_duration_min.value() * SECONDS_IN_MINUTE;

Check notice

Code scanning / Cppcheck (reported by Codacy)

MISRA 15.5 rule Note

MISRA 15.5 rule
};

tbb::parallel_for_each(
m_trajectories.begin(),
m_trajectories.end(),
[&to_remove,
&to_split,
check_max_speed,
check_min_duration,
&check_max_speed,

Check notice

Code scanning / Cppcheck (reported by Codacy)

MISRA 12.3 rule Note

MISRA 12.3 rule
&check_min_duration,

Check notice

Code scanning / Cppcheck (reported by Codacy)

MISRA 12.3 rule Note

MISRA 12.3 rule
min_points_per_trajectory,
cluster_radius_km,
max_speed_kph,
Expand All @@ -142,16 +149,16 @@
return;
}
auto const& points{trajectory.points()};
for (std::size_t i = 0; i < points.size(); ++i) {
auto const nPoints = points.size();
for (std::size_t i = 0; i < nPoints;) {
auto const& currentCluster = points[i];
if (min_duration_min.has_value() &&
!check_min_duration(currentCluster, min_duration_min.value())) {
if (!check_min_duration(currentCluster)) {
to_split.insert(uid);
return;
}
if (i > 0) {
auto const& previousCluster = points[i - 1];
if (!check_max_speed(currentCluster, previousCluster, max_speed_kph)) {
if (++i < nPoints) {
auto const& nextCluster = points[i];
if (!check_max_speed(nextCluster, currentCluster)) {
to_split.insert(uid);
return;
}
Expand All @@ -169,8 +176,10 @@
return to_remove.contains(pair.first);
});

spdlog::info("Splitting {} trajectories based on minimum duration requirement.",
to_split.size());
spdlog::info(
"Splitting {} trajectories based on minimum duration or maximum speed "
"requirements.",
to_split.size());
for (auto const& uid : to_split) {
// Extract the trajectory
if (!m_trajectories.contains(uid)) {
Expand All @@ -182,18 +191,19 @@

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

bool bShouldSplit = false;

if (i > 0) {
auto const& previousCluster = points[i - 1];
bShouldSplit = !check_max_speed(currentCluster, previousCluster, max_speed_kph);
if (++i < nPoints) {
auto const& nextCluster = points[i];
bShouldSplit = !check_max_speed(nextCluster, currentCluster);
}
if (min_duration_min.has_value() && !bShouldSplit) {
bShouldSplit = !check_min_duration(currentCluster, min_duration_min.value());
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()) {
Expand Down
Loading