-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
Distances in matrix - CH #4990
Distances in matrix - CH #4990
Conversation
Context: Within To calculate the distance, I'm trying to use HaversineDistance to calculate the distances between two NodeIDs here in this Question: the difficulty I'm having is in supplying the second Node ID (the third argument to What I've tried: If I pass in the second NodeID available to me within the
From the output above, it looks like we want the OSRM node id that matches the node ids in the m_coordinate_list. However, if the start of the edge or the end of the edge is the
|
We need to do the following things:
|
I did this and it's working these are the results I have so far:
for this scenario:
@TheMarex so I think what's left now is the offset calculations. I looked at this a bit, and I see that we use So to get the distance offsets, is the extractor code next in line to be augmented? Maybe in the Or is this the wrong track? 😄 |
We can replicate this logic and loop over all node ids up until the phantom node segment and then do a similar calculation as the one you implemented for the annotations to sum up the distances. One particular snag is that we need to add the distance to the phantom node location. Suppose we have a phantom node that is snapped to an edge-based-node that consists of four segments:
The whole distance of the edge-based-node would be the haversine distance of the node-based-nodes |
Another thought - we need to ensure the phantom node portions don't get cached :-/ |
@danpat we don't put that part of the calculation in the cache. Instead we do those offset calculations in the main body of the many_to_many function. For instance, for durations, those calculations happen here: https://github.com/Project-OSRM/osrm-backend/pull/4876/files#diff-81e2dc0f0a96c3d1a3fd7e43ee98a7f3R328 All the cache stuff happens 7 lines above, over here: https://github.com/Project-OSRM/osrm-backend/pull/4876/files#diff-81e2dc0f0a96c3d1a3fd7e43ee98a7f3R321 |
Okay so with the latest commit: ea8cdc7 I'm running into this compile error that I suspect comes from packing problems.
The datatype that I've added Questions:
The other problem is that no matter where in the PhantomNode constructor, I place the new data members, the compiler keeps warning me about the order in which the data members will be initialized in.
|
Oookay, got the answers:
All these problems are fixed for now! |
770e531
to
f4ebd2d
Compare
Okay, so here's the latest, now that I've added put the offset functions into use:
I get this table:
I have not yet debugged these results other than a check to make sure that the distance offsets exactly matches all the offsetting behaviour for durations. This may be a wrong assumption, but I will tune that assumption later when I debug. |
fb7f3bb
to
40736a5
Compare
3bfdaf3
to
729a399
Compare
Distances on CH works! I've built out the pipeline through to the node bindings and have added cucumber and table tests. Here are a few:
@TheMarex if you'd give this a review that would be good. |
e3106c9
to
582ed3b
Compare
Ok. Trying again. Ran 5000
Average time on current
|
More numbers on matrices requests with
|
More numbers on matrices requests with
|
README.md
Outdated
@@ -9,7 +9,7 @@ High performance routing engine written in C++14 designed to run on OpenStreetMa | |||
The following services are available via HTTP API, C++ library interface and NodeJs wrapper: | |||
- Nearest - Snaps coordinates to the street network and returns the nearest matches | |||
- Route - Finds the fastest route between coordinates | |||
- Table - Computes the duration of the fastest route between all pairs of supplied coordinates | |||
- Table - Computes the duration of the fastest route between all pairs of supplied coordinates, and optionally returns distances of these routes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just shorten this to
- Table - Computes the duration or distances of the fastest route between all pairs of supplied coordinates
The other services don't list all their options either.
docs/http.md
Outdated
@@ -222,20 +222,21 @@ curl 'http://router.project-osrm.org/route/v1/driving/13.388860,52.517037;13.397 | |||
|
|||
### Table service | |||
|
|||
Computes the duration of the fastest route between all pairs of supplied coordinates. Also returns the distances between the coordinate pairs. Note that the distances are not the shortest distance between two coordinates, but rather the distances of the fastest routes. | |||
Computes the duration of the fastest route between all pairs of supplied coordinates. Optinally, also returns the distances between the coordinate pairs. Note that the distances are not the shortest distance between two coordinates, but rather the distances of the fastest routes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo Optionally
docs/http.md
Outdated
|
||
In addition to the [general options](#general-options) the following options are supported for this service: | ||
|
||
|Option |Values |Description | | ||
|------------|--------------------------------------------------|---------------------------------------------| | ||
|sources |`{index};{index}[;{index} ...]` or `all` (default)|Use location with given index as source. | | ||
|destinations|`{index};{index}[;{index} ...]` or `all` (default)|Use location with given index as destination.| | ||
|annotations |`duration` (default), `distance` |Return additional table with distances to the response. Whether requested or not, the duration table is always returned.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duration
(default), distance
or duration,distance
maybe?
docs/http.md
Outdated
|
||
# Returns a 3x3 duration matrix and a 3x3 distance matrix: | ||
curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397634,52.529407;13.428555,52.523219&annotations=distance' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok wait. no. This is confusing. so
annotation=distance
and annotation=duration,distance
will return the same response?
If yes, and this is intended we should make it clearer. this is confusing.
If yes, and this is not 100% thought through can we discuss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm - I think the expected thing is that there would only be a distances
field for passing annotations=distance
and both for annotations=duration,distance
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheMarex actually maybe you can clarify this. In the comment here: #4990 (comment) I took this to mean that we should always return duration
table whether it was asked for or not in the response. That's what I've implemented and what I've documented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks almost perfect to me. 💯 There are only a few nitpicks we could fix, and some questions. Good work. 👍
CHANGELOG.md
Outdated
@@ -1,3 +1,8 @@ | |||
# UNRELEASED | |||
- Features: | |||
- ADDED: `table` plugin now optionally returns `distance` matrix as part of response [#4990](https://github.com/Project-OSRM/osrm-backend/pull/4990) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
distance
-> distances
(the analog property is also called durations
)
docs/http.md
Outdated
|
||
# Returns a 3x3 duration matrix and a 3x3 distance matrix: | ||
curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397634,52.529407;13.428555,52.523219&annotations=distance' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm - I think the expected thing is that there would only be a distances
field for passing annotations=distance
and both for annotations=duration,distance
, no?
@@ -0,0 +1,492 @@ | |||
@matrix @testbot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the renaming, good by "distance" table. 😹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
| a | 0 | 10 | | ||
| b | 10 | 0 | | ||
|
||
@ch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm do you know why this is marked CH
only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests fail on the mld profile 😬 . Going to ticket out as a bug!
| c | | | 0 | 10 | | ||
| d | | | 10 | 0 | | ||
|
||
@ch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
|
||
relaxOutgoingEdges<REVERSE_DIRECTION>( | ||
facade, node, target_weight, target_duration, query_heap, phantom_node); | ||
} | ||
|
||
} // namespace ch | ||
|
||
void retrievePackedPathFromSearchSpace(NodeID middle_node_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: middle_node_id
can be const
.
const DataFacade<ch::Algorithm> &facade, | ||
const std::vector<PhantomNode> &phantom_nodes, | ||
const std::vector<std::size_t> &target_indices, | ||
const unsigned row_idx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: We try to avoid abbreviations like idx
and prefer index
.
const unsigned row_idx, | ||
const std::size_t source_index, | ||
const PhantomNode &source_phantom, | ||
const unsigned number_of_targets, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make number_of_targets
and row_index
a std::size_t
for consistency (no real impact on performance or anything just a style thing).
{ | ||
std::vector<NodeID> packed_leg; | ||
|
||
for (unsigned column_idx = 0; column_idx < number_of_targets; ++column_idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be simplified to for (auto column_index : util::irange<std::size_t>(0, number_of_targets))
coordinates: [three_test_coordinates[0], three_test_coordinates[1]], | ||
sources: [0], | ||
destinations: [0,1], | ||
annotations: [annotation.slice(0,-1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that our annotations parameters uses singular over plural is one of my biggest API regrets. 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
ca07b47
to
649e0af
Compare
Release OSRM 5.18.0 Changes from 5.17.0: - Features: - ADDED: `table` plugin now optionally returns `distance` matrix as part of response [Project-OSRM#4990](Project-OSRM#4990) - ADDED: New optional parameter `annotations` for `table` that accepts `distance`, `duration`, or both `distance,duration` as values [Project-OSRM#4990](Project-OSRM#4990) - Infrastructure: - ADDED: Updated libosmium and added protozero and vtzero libraries [Project-OSRM#5037](Project-OSRM#5037) - CHANGED: Use vtzero library in tile plugin [Project-OSRM#4686](Project-OSRM#4686) - Profile: - ADDED: Bicycle profile now returns classes for ferry and tunnel routes. [Project-OSRM#5054](Project-OSRM#5054) - ADDED: Bicycle profile allows to exclude ferry routes (default to not enabled) [Project-OSRM#5054](Project-OSRM#5054)
Issue
This PR implements distances in the matrix as in this issue: #1353
Tasklist
Requirements / Relations
The unpacking cache in #4876 needs to be merged in to master first.This is currently shipping without the cache.Current speeds are described here.
However, @chaupow and @danpat are cooking up faster distance calculations using cheap-ruler over in #5041!