Dedup coordinate in OSRM queries - #366
Conversation
Walkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ad8e058 to
cfdf846
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/osrmgeofilter.cpp (1)
91-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle OSRM N×1 response shapes when
reversed=true.
&destinations=0makes OSRM return one row per source, soresponseJson["durations"][0]is a single-element row. The current size check expectsnextOsrmPositioncolumns and reads every node from row 0, so any reverse lookup with one or more nearby nodes returns an empty vector. Use the row count forreversedpaths and index[i][0]in the read loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/osrmgeofilter.cpp` around lines 91 - 98, Update the reversed OSRM response handling around the queryString destination selection to use the response row count for size validation instead of nextOsrmPosition. In the duration-reading loop, preserve the existing indexing for forward paths but read reversed N×1 responses from durations[i][0], allowing all nearby nodes to be returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/osrmgeofilter.cpp`:
- Around line 91-98: Update the reversed OSRM response handling around the
queryString destination selection to use the response row count for size
validation instead of nextOsrmPosition. In the duration-reading loop, preserve
the existing indexing for forward paths but read reversed N×1 responses from
durations[i][0], allowing all nearby nodes to be returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aedfcabd-b39c-482a-b912-387501d10955
📒 Files selected for processing (1)
src/osrmgeofilter.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Analyze (cpp)
- GitHub Check: Test build on macOS-latest (memcached: true)
- GitHub Check: Test build on ubuntu-latest (memcached: true)
- GitHub Check: Test build on macOS-latest (memcached: false)
- GitHub Check: Test build on ubuntu-latest (memcached: false)
- GitHub Check: pr-build-check
🧰 Additional context used
🪛 Clang (14.0.6)
src/osrmgeofilter.cpp
[warning] 18-18: constructor does not initialize these fields: osrmPosition
(cppcoreguidelines-pro-type-member-init)
[warning] 39-39: variable 'potentialAccessibleNodes' is not initialized
(cppcoreguidelines-init-variables)
[warning] 44-44: variable 'osrmPositionByCoordinates' is not initialized
(cppcoreguidelines-init-variables)
[warning] 135-135: variable 'travelTimeSeconds' is not initialized
(cppcoreguidelines-init-variables)
[warning] 136-136: variable 'distanceMeters' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
src/osrmgeofilter.cpp
[style] 17-17: The function 'asWKT' is never used.
(unusedFunction)
[style] 44-44: The function 'toString' is never used.
(unusedFunction)
[style] 143-143: The function 'status_code' is never used.
(unusedFunction)
🔇 Additional comments (2)
src/osrmgeofilter.cpp (2)
1-4: LGTM!Also applies to: 14-23
37-47: LGTM!Dedup logic (map coordinate string → column, first-seen wins) is correctly wired into
PotentialAccessibleNode, and the empty-check now correctly targetspotentialAccessibleNodes.Also applies to: 67-83
|
I've create #367 for the rabbit comment, but it's for a code path that is not used at all for the moment. |
|
Some numbers. (not a huge win, but seeing a few calc/sec improvement is still good) |
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Sounds good, just a few comments and question
| bool reversed) | ||
| { | ||
| std::vector<std::reference_wrapper<const Node>> birdDistanceAccessibleNodeIndexes; | ||
| // All the nodes within bird distance, in nodes-map order, as before, |
There was a problem hiding this comment.
"as before" before what? and what's a nodes-map order? Is it relevant?
| nextOsrmPosition++; | ||
| } | ||
|
|
||
| potentialAccessibleNodes.push_back({std::cref(node), inserted.first->second}); |
There was a problem hiding this comment.
Maybe just a comment here or at the if explaining quickly that inserted.first->second is the position corresponding to the effective coordinates, just so one does not have to go read the emplace doc, like i just did.
| { | ||
| birdDistanceAccessibleNodeIndexes.push_back(node); | ||
| queryString += ";" + std::to_string(node.point.get()->longitude) + "," + std::to_string(node.point.get()->latitude); | ||
| std::string coordinates = std::to_string(node.point.get()->longitude) + "," + std::to_string(node.point.get()->latitude); |
There was a problem hiding this comment.
When does the 6 decimals rounding take place?
There was a problem hiding this comment.
in the to_string
| // co-located nodes share a column | ||
| for (const auto & potentialNode : potentialAccessibleNodes) | ||
| { | ||
| std::size_t i = potentialNode.osrmPosition; |
There was a problem hiding this comment.
maybe you could use a variable name clearer than i? This is not a for loop anymore
cfdf846 to
6b985b9
Compare
We have case where Transition have nodes with similar or exact same coordinate. (Probably from multiple GTFS import where node merging was disabled.) That causes OSRM queries to take longer. By deduplicating, we reduce significantly the processing time. For example: 106 -> 54 orig 50.60 ms dedup 27.87 ms (saving 22 ms) 134 -> 84 orig 81.34 ms dedup 50.67 ms (saving 30 ms) To do so, we keep the list of potential accessible nodes as filtering by euclidean distance with an index pointing to it's position in the OSRM query. We keep a map of the query coordinates converted as string to find the duplication. We then check for each potential nodes if we had a results and generate the resulting accessible nodes list. Also added a convenience function to convert a Point object to the expect OSRM string format
6b985b9 to
4c3a44f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/osrmgeofilter.cpp`:
- Around line 14-29: Add a unit test covering formatOsrmCoordinates that
verifies both longitude and latitude are rendered with the current six-decimal
std::to_string precision, including trailing zeros where applicable. Keep the
test focused on the formatting contract used by the OSRM destination
deduplication logic.
- Around line 147-167: Cache the first response arrays from
responseJson["durations"] and responseJson["distances"] in local references
before the potentialAccessibleNodes loop, then use those cached references for
null checks and value conversions inside the loop. Keep the existing filtering
and logging behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d9592ff-38f4-4469-b7fc-ffa8d83774c8
📒 Files selected for processing (1)
src/osrmgeofilter.cpp
📜 Review details
🧰 Additional context used
🪛 Clang (14.0.6)
src/osrmgeofilter.cpp
[warning] 18-18: constructor does not initialize these fields: osrmPosition
(cppcoreguidelines-pro-type-member-init)
[warning] 25-25: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 45-45: variable 'potentialAccessibleNodes' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'osrmPositionByCoordinates' is not initialized
(cppcoreguidelines-init-variables)
[warning] 143-143: variable 'travelTimeSeconds' is not initialized
(cppcoreguidelines-init-variables)
[warning] 144-144: variable 'distanceMeters' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
src/osrmgeofilter.cpp
[style] 17-17: The function 'asWKT' is never used.
(unusedFunction)
[style] 44-44: The function 'toString' is never used.
(unusedFunction)
[style] 52-52: The function 'getMaxUid' is never used.
(unusedFunction)
[style] 143-143: The function 'status_code' is never used.
(unusedFunction)
🔇 Additional comments (6)
src/osrmgeofilter.cpp (6)
1-4: LGTM!
43-53: LGTM!
62-62: LGTM!
73-91: LGTM!
168-171: LGTM!
128-141: 🎯 Functional CorrectnessNo change needed.
The current callers for
OsrmGeoFilteruse the defaultreversed=false, so thereversed=truepath is not reached; the API also interpretssources=0/destinations=0as coordinate index 0 rather than a count.
| namespace { | ||
| /* A node that passed the bird distance filter, paired with the column it | ||
| maps to in the OSRM table response. Nodes sharing coordinates share a | ||
| column, so this is not simply the node's rank in the request. */ | ||
| struct PotentialAccessibleNode { | ||
| std::reference_wrapper<const Node> node; | ||
| std::size_t osrmPosition; | ||
| }; | ||
|
|
||
| // By default, to_string convert a double to a string with 6 decimal digits | ||
| // TODO Add a unit test to ensure the formatting always have the right precisions (default might change in future standards) | ||
| std::string formatOsrmCoordinates(const Point &point) { | ||
| return std::to_string(point.longitude) + "," + std::to_string(point.latitude); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a unit test for coordinate formatting precision.
The comment at Line 23 and the TODO at Line 24 flag that std::to_string precision for coordinates is not covered by a test. This precision directly drives the deduplication logic below, so a change in default formatting would silently alter which nodes get merged into one OSRM destination.
Do you want me to draft a unit test for formatOsrmCoordinates that pins the current 6-decimal behavior?
🧰 Tools
🪛 Clang (14.0.6)
[warning] 18-18: constructor does not initialize these fields: osrmPosition
(cppcoreguidelines-pro-type-member-init)
[warning] 25-25: use a trailing return type for this function
(modernize-use-trailing-return-type)
🪛 Cppcheck (2.21.0)
[style] 17-17: The function 'asWKT' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/osrmgeofilter.cpp` around lines 14 - 29, Add a unit test covering
formatOsrmCoordinates that verifies both longitude and latitude are rendered
with the current six-decimal std::to_string precision, including trailing zeros
where applicable. Keep the test focused on the formatting contract used by the
OSRM destination deduplication logic.
| for (const auto & potentialNode : potentialAccessibleNodes) | ||
| { | ||
| std::size_t position = potentialNode.osrmPosition; | ||
|
|
||
| // Check if the duration and distance values are null before attempting to convert them | ||
| if (!responseJson["durations"][0][position].is_null() && !responseJson["distances"][0][position].is_null()) | ||
| { | ||
| // Check if the duration and distance values are null before attempting to convert them | ||
| if (!responseJson["durations"][0][i].is_null() && !responseJson["distances"][0][i].is_null()) | ||
| travelTimeSeconds = (int)ceil((float)responseJson["durations"][0][position]); | ||
| if (travelTimeSeconds <= maxWalkingTravelTime) | ||
| { | ||
| travelTimeSeconds = (int)ceil((float)responseJson["durations"][0][i]); | ||
| if (travelTimeSeconds <= maxWalkingTravelTime) | ||
| { | ||
| distanceMeters = (int)ceil((float)responseJson["distances"][0][i]); | ||
| accessibleNodesFootpaths.push_back(NodeTimeDistance(birdDistanceAccessibleNodeIndexes[i - 1], | ||
| travelTimeSeconds, | ||
| distanceMeters)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| spdlog::debug("skipping node at index {} due to null duration or distance from OSRM", i); | ||
| distanceMeters = (int)ceil((float)responseJson["distances"][0][position]); | ||
| accessibleNodesFootpaths.push_back(NodeTimeDistance(potentialNode.node, | ||
| travelTimeSeconds, | ||
| distanceMeters)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| spdlog::debug("skipping node at index {} due to null duration or distance from OSRM", position); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Cache the response arrays outside the loop.
responseJson["durations"][0] and responseJson["distances"][0] are looked up by key on every iteration of the loop, for every candidate node. Since the PR's goal is to reduce per-request overhead, cache these two references once before the loop and index only with position inside it.
⚡ Proposed caching of response arrays
int travelTimeSeconds;
int distanceMeters;
+ const auto &durationsRow = responseJson["durations"][0];
+ const auto &distancesRow = responseJson["distances"][0];
// Iterate over the nodes rather than over the response columns, since
// co-located nodes share a column
for (const auto & potentialNode : potentialAccessibleNodes)
{
std::size_t position = potentialNode.osrmPosition;
// Check if the duration and distance values are null before attempting to convert them
- if (!responseJson["durations"][0][position].is_null() && !responseJson["distances"][0][position].is_null())
+ if (!durationsRow[position].is_null() && !distancesRow[position].is_null())
{
- travelTimeSeconds = (int)ceil((float)responseJson["durations"][0][position]);
+ travelTimeSeconds = (int)ceil((float)durationsRow[position]);
if (travelTimeSeconds <= maxWalkingTravelTime)
{
- distanceMeters = (int)ceil((float)responseJson["distances"][0][position]);
+ distanceMeters = (int)ceil((float)distancesRow[position]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const auto & potentialNode : potentialAccessibleNodes) | |
| { | |
| std::size_t position = potentialNode.osrmPosition; | |
| // Check if the duration and distance values are null before attempting to convert them | |
| if (!responseJson["durations"][0][position].is_null() && !responseJson["distances"][0][position].is_null()) | |
| { | |
| // Check if the duration and distance values are null before attempting to convert them | |
| if (!responseJson["durations"][0][i].is_null() && !responseJson["distances"][0][i].is_null()) | |
| travelTimeSeconds = (int)ceil((float)responseJson["durations"][0][position]); | |
| if (travelTimeSeconds <= maxWalkingTravelTime) | |
| { | |
| travelTimeSeconds = (int)ceil((float)responseJson["durations"][0][i]); | |
| if (travelTimeSeconds <= maxWalkingTravelTime) | |
| { | |
| distanceMeters = (int)ceil((float)responseJson["distances"][0][i]); | |
| accessibleNodesFootpaths.push_back(NodeTimeDistance(birdDistanceAccessibleNodeIndexes[i - 1], | |
| travelTimeSeconds, | |
| distanceMeters)); | |
| } | |
| } | |
| else | |
| { | |
| spdlog::debug("skipping node at index {} due to null duration or distance from OSRM", i); | |
| distanceMeters = (int)ceil((float)responseJson["distances"][0][position]); | |
| accessibleNodesFootpaths.push_back(NodeTimeDistance(potentialNode.node, | |
| travelTimeSeconds, | |
| distanceMeters)); | |
| } | |
| } | |
| else | |
| { | |
| spdlog::debug("skipping node at index {} due to null duration or distance from OSRM", position); | |
| } | |
| } | |
| int travelTimeSeconds; | |
| int distanceMeters; | |
| const auto &durationsRow = responseJson["durations"][0]; | |
| const auto &distancesRow = responseJson["distances"][0]; | |
| for (const auto & potentialNode : potentialAccessibleNodes) | |
| { | |
| std::size_t position = potentialNode.osrmPosition; | |
| // Check if the duration and distance values are null before attempting to convert them | |
| if (!durationsRow[position].is_null() && !distancesRow[position].is_null()) | |
| { | |
| travelTimeSeconds = (int)ceil((float)durationsRow[position]); | |
| if (travelTimeSeconds <= maxWalkingTravelTime) | |
| { | |
| distanceMeters = (int)ceil((float)distancesRow[position]); | |
| accessibleNodesFootpaths.push_back(NodeTimeDistance(potentialNode.node, | |
| travelTimeSeconds, | |
| distanceMeters)); | |
| } | |
| } | |
| else | |
| { | |
| spdlog::debug("skipping node at index {} due to null duration or distance from OSRM", position); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/osrmgeofilter.cpp` around lines 147 - 167, Cache the first response
arrays from responseJson["durations"] and responseJson["distances"] in local
references before the potentialAccessibleNodes loop, then use those cached
references for null checks and value conversions inside the loop. Keep the
existing filtering and logging behavior unchanged.
2 participants
We have case where Transition have nodes with similar or exact same coordinate. (Probably from multiple GTFS import where node merging was disabled.)
That causes OSRM queries to take longer. By deduplicating, we reduce significantly the processing time. For example:
106 -> 54 orig 50.60 ms dedup 27.87 ms (saving 22 ms)
134 -> 84 orig 81.34 ms dedup 50.67 ms (saving 30 ms)
To do so, we keep the list of potential accessible nodes as filtering by euclidean distance with an index pointing to it's position in the OSRM query. We keep a map of the query coordinates converted as string to find the duplication.
We then check for each potential nodes if we had a results and generate the resulting accessible nodes list.
Summary by CodeRabbit