Skip to content
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

Support rectangular matrix with less sources than targets #1760

Closed
wants to merge 2 commits into from
Closed

Support rectangular matrix with less sources than targets #1760

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
e66ab6e
Support rectangular matrix with less sources than targets
fab-girard Nov 5, 2015
3cc75c0
Use now 3 parameters for matrix: loc/dst/src
fab-girard Dec 2, 2015
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
20 changes: 19 additions & 1 deletion data_structures/route_parameters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

RouteParameters::RouteParameters()
: zoom_level(18), print_instructions(false), alternate_route(true), geometry(true),
compression(true), deprecatedAPI(false), uturn_default(false), classify(false),
compression(true), deprecatedAPI(false), uturn_default(false), classify(false), mapped_points(true),
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the mapped_points parameter entirely. It has no significant implications on size or performance. I'm only talking about the parameter for the API not the feature as such - it should just be the normal behavior and not user configurable.

Copy link
Member

Choose a reason for hiding this comment

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

This as option was made to avoid network transfer of data, if not needed by client. With huge matrix in mind.

Copy link
Member

Choose a reason for hiding this comment

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

For really huge matrices you should avoid going over the network (uncompressed) anyway. You can take a look at our node-osrm bindings. Since this is all 100% numerical values you can get a way with about 4 bytes per entry (just save the raw matrix and the size).

matching_beta(5), gps_precision(5), check_sum(-1), num_results(1)
{
}
Expand Down Expand Up @@ -148,8 +148,26 @@ void RouteParameters::addCoordinate(
static_cast<int>(COORDINATE_PRECISION * boost::fusion::at_c<1>(received_coordinates)));
}

void RouteParameters::addDestination(
const boost::fusion::vector<double, double> &received_coordinates)
{
destinations.emplace_back(
static_cast<int>(COORDINATE_PRECISION * boost::fusion::at_c<0>(received_coordinates)),
static_cast<int>(COORDINATE_PRECISION * boost::fusion::at_c<1>(received_coordinates)));
}

void RouteParameters::addSource(
const boost::fusion::vector<double, double> &received_coordinates)
{
sources.emplace_back(
static_cast<int>(COORDINATE_PRECISION * boost::fusion::at_c<0>(received_coordinates)),
static_cast<int>(COORDINATE_PRECISION * boost::fusion::at_c<1>(received_coordinates)));
}

void RouteParameters::getCoordinatesFromGeometry(const std::string &geometry_string)
{
PolylineCompressor pc;
coordinates = pc.decode_string(geometry_string);
}

void RouteParameters::setMappedPointsFlag(const bool flag) { mapped_points = flag; }
14 changes: 9 additions & 5 deletions features/step_definitions/distance_matrix.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
When /^I request a travel time matrix I should get$/ do |table|

no_route = 2147483647 # MAX_INT

raise "*** Top-left cell of matrix table must be empty" unless table.headers[0]==""

nodes = []
sources = []
column_headers = table.headers[1..-1]
row_headers = table.rows.map { |h| h.first }
unless column_headers==row_headers
raise "*** Column and row headers must match in matrix table, got #{column_headers.inspect} and #{row_headers.inspect}"
end
column_headers.each do |node_name|
node = find_node_by_name(node_name)
raise "*** unknown node '#{node_name}" unless node
nodes << node
end
if column_headers!=row_headers
row_headers.each do |node_name|
node = find_node_by_name(node_name)
raise "*** unknown node '#{node_name}" unless node
sources << node
end
end

reprocess
actual = []
Expand All @@ -23,7 +27,7 @@

# compute matrix
params = @query_params
response = request_table nodes, params
response = request_table nodes, sources, params
if response.body.empty? == false
json = JSON.parse response.body
result = json['distance_table']
Expand Down
8 changes: 6 additions & 2 deletions features/support/route.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,13 @@ def request_route waypoints, params={}
request_path "viaroute", waypoints, defaults.merge(params)
end

def request_table waypoints, params={}
def request_table waypoints, sources, params={}
defaults = { 'output' => 'json' }
request_path "table", waypoints, defaults.merge(params)
options = defaults.merge(params)
locs = (sources.size > 0) ? (waypoints.compact.map { |w| "dst=#{w.lat},#{w.lon}" } + sources.compact.map { |w| "src=#{w.lat},#{w.lon}" }) : waypoints.compact.map { |w| "loc=#{w.lat},#{w.lon}" }
params = (locs + options.to_param).join('&')
uri = generate_request_url ("table" + '?' + params)
response = send_request uri, waypoints, options, sources
end

def got_route? response
Expand Down
38 changes: 38 additions & 0 deletions features/testbot/distance_matrix.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,41 @@ Feature: Basic Distance Matrix
| y | 500 | 0 | 300 | 200 |
| d | 200 | 300 | 0 | 300 |
| e | 300 | 400 | 100 | 0 |

Scenario: Testbot - Travel time matrix and mapped coordinates with only one source
Given the node map
| a | b | c |
| d | e | f |

And the ways
| nodes |
| abc |
| def |
| ad |
| be |
| cf |

And the query options
| mapped_points | true |

When I request a travel time matrix I should get
| | a | b | e | f |
| a | 0 | 100 | 200 | 300 |

Scenario: Testbot - Travel time 3x2 matrix
Given the node map
| a | b | c |
| d | e | f |

And the ways
| nodes |
| abc |
| def |
| ad |
| be |
| cf |

When I request a travel time matrix I should get
| | b | e | f |
| a | 100 | 200 | 300 |
| b | 0 | 100 | 200 |
9 changes: 9 additions & 0 deletions include/osrm/route_parameters.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,14 @@ struct RouteParameters

void addCoordinate(const boost::fusion::vector<double, double> &received_coordinates);

void addDestination(const boost::fusion::vector<double, double> &received_coordinates);

void addSource(const boost::fusion::vector<double, double> &received_coordinates);

void getCoordinatesFromGeometry(const std::string &geometry_string);

void setMappedPointsFlag(const bool flag);

short zoom_level;
bool print_instructions;
bool alternate_route;
Expand All @@ -92,6 +98,7 @@ struct RouteParameters
bool deprecatedAPI;
bool uturn_default;
bool classify;
bool mapped_points;
double matching_beta;
double gps_precision;
unsigned check_sum;
Expand All @@ -105,6 +112,8 @@ struct RouteParameters
std::vector<std::pair<const int,const boost::optional<int>>> bearings;
std::vector<bool> uturns;
std::vector<FixedPointCoordinate> coordinates;
std::vector<FixedPointCoordinate> destinations;
std::vector<FixedPointCoordinate> sources;
};

#endif // ROUTE_PARAMETERS_HPP
96 changes: 79 additions & 17 deletions plugins/distance_table.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,24 +70,34 @@ template <class DataFacadeT> class DistanceTablePlugin final : public BasePlugin
int HandleRequest(const RouteParameters &route_parameters,
osrm::json::Object &json_result) override final
{
if (!check_all_coordinates(route_parameters.coordinates))
const bool useSameTgtSrc = route_parameters.coordinates.size() ? true : false;
if ((useSameTgtSrc && route_parameters.destinations.size()) || (useSameTgtSrc && route_parameters.sources.size()))
{
return 400;
}

if ((useSameTgtSrc && !check_all_coordinates(route_parameters.coordinates)) ||
(!useSameTgtSrc && !check_all_coordinates(route_parameters.destinations, 2) && !check_all_coordinates(route_parameters.sources, 1)))
Copy link
Member

Choose a reason for hiding this comment

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

useSameTgtSrc is unneeded. This can be replaced with:

if (!check_all_coordinates(route_parameter.coordinates) || !check_all_coordinates(route_parameter.sources) || !check_all_coordinates(route_parameters.destinations)) { return 400; }

{
return 400;
}

const auto &input_bearings = route_parameters.bearings;
if (input_bearings.size() > 0 && route_parameters.coordinates.size() != input_bearings.size())
unsigned nb_coordinates = useSameTgtSrc ? route_parameters.coordinates.size() : (route_parameters.destinations.size() + route_parameters.sources.size());
Copy link
Member

Choose a reason for hiding this comment

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

  1. Don't use abbreviations in variable names.
  2. Expected logic would be: auto number_of_coordinates = route_parameters.coordinates.size() + route_parameters.sources.size() + route_parameters.destinations.size();

if (input_bearings.size() > 0 && nb_coordinates != input_bearings.size())
{
json_result.values["status"] = "Number of bearings does not match number of coordinates .";
return 400;
}

const bool checksum_OK = (route_parameters.check_sum == facade->GetCheckSum());
unsigned max_locations =
unsigned max_locations = useSameTgtSrc ?
Copy link
Member

Choose a reason for hiding this comment

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

I guess it would be cleaner to just return 400 for tables that exceed a size of max_locations_distance_table*max_locations_distance_table so this should be more like:

if (number_of_sources*number_of_destinations > max_locations_distance_table*max_locations_distance_table)
{
    json_result.values["status_message"] = std::string("Table exceeds maximum number of entries of ") + std::to_string(max_locations_distance_table*max_locations_distance_table);
    return 400;
}

Where auto number_of_sources = route_parameters.coordinates.size() + route_parameters.sources.size() and auto number_of_destinations = route_parameters.coordinates.size() + route_parameters.destinations.size();

std::min(static_cast<unsigned>(max_locations_distance_table),
static_cast<unsigned>(route_parameters.coordinates.size())) :
std::min(static_cast<unsigned>(max_locations_distance_table),
static_cast<unsigned>(route_parameters.coordinates.size()));
static_cast<unsigned>(route_parameters.destinations.size()));

PhantomNodeArray phantom_node_vector(max_locations);
PhantomNodeArray phantom_node_target_vector(max_locations);
Copy link
Member

Choose a reason for hiding this comment

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

max_locations is wrong here as it would be number_of_destinations.

for (const auto i : osrm::irange(0u, max_locations))
Copy link
Member

Choose a reason for hiding this comment

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

You can move this loop to a utility function and like this:

GetCoordinates(const std::vector<FixedPointCoordinate>& inputs, PhantomNodeArray& output)
{
....
}

The code should then look somethings like:

GetCoordinates(route_parameters.coordinates, phantom_node_vector);
std::insert(phantom_node_source_vector.end(), phantom_node_vector.begin(), phantom_node_vector.end());
std::insert(phantom_node_destination_vector.end(), phantom_node_vector.begin(), phantom_node_vector.end());
GetCoordinates(route_parameters.sources, phantom_node_source_vector);
GetCoordinates(route_parameters.destinations, phantom_node_destination_vector);

{
if (checksum_OK && i < route_parameters.hints.size() &&
Expand All @@ -97,39 +107,91 @@ template <class DataFacadeT> class DistanceTablePlugin final : public BasePlugin
ObjectEncoder::DecodeFromBase64(route_parameters.hints[i], current_phantom_node);
if (current_phantom_node.is_valid(facade->GetNumberOfNodes()))
{
phantom_node_vector[i].emplace_back(std::move(current_phantom_node));
phantom_node_target_vector[i].emplace_back(std::move(current_phantom_node));
continue;
}
}
const int bearing = input_bearings.size() > 0 ? input_bearings[i].first : 0;
const int range = input_bearings.size() > 0 ? (input_bearings[i].second?*input_bearings[i].second:10) : 180;
facade->IncrementalFindPhantomNodeForCoordinate(route_parameters.coordinates[i],
phantom_node_vector[i], 1, bearing, range);
facade->IncrementalFindPhantomNodeForCoordinate(useSameTgtSrc ? route_parameters.coordinates[i] : route_parameters.destinations[i],
phantom_node_target_vector[i], 1, bearing, range);

BOOST_ASSERT(phantom_node_vector[i].front().is_valid(facade->GetNumberOfNodes()));
BOOST_ASSERT(phantom_node_target_vector[i].front().is_valid(facade->GetNumberOfNodes()));
}
unsigned shift_coordinates = (useSameTgtSrc) ? 0 : route_parameters.destinations.size();
max_locations = 0;
if (!useSameTgtSrc)
{
max_locations = std::min(static_cast<unsigned>(max_locations_distance_table),
static_cast<unsigned>(route_parameters.sources.size()));
}
PhantomNodeArray phantom_node_source_vector(max_locations);
for (const auto i : osrm::irange(0u, max_locations))
{
if (checksum_OK && i < route_parameters.hints.size() - shift_coordinates &&
!route_parameters.hints[i + shift_coordinates].empty())
{
PhantomNode current_phantom_node;
ObjectEncoder::DecodeFromBase64(route_parameters.hints[i + shift_coordinates], current_phantom_node);
Copy link
Member

Choose a reason for hiding this comment

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

We sadly need to drop hint support here. 5.0.0 will restructure the API a little, I think we can live with this for one release cycle.

if (current_phantom_node.is_valid(facade->GetNumberOfNodes()))
{
phantom_node_source_vector[i].emplace_back(std::move(current_phantom_node));
continue;
}
}
const int bearing = input_bearings.size() > 0 ? input_bearings[i + shift_coordinates].first : 0;
const int range = input_bearings.size() > 0 ? (input_bearings[i + shift_coordinates].second?*input_bearings[i + shift_coordinates].second:10) : 180;
facade->IncrementalFindPhantomNodeForCoordinate(route_parameters.sources[i],
phantom_node_source_vector[i], 1, bearing, range);

BOOST_ASSERT(phantom_node_source_vector[i].front().is_valid(facade->GetNumberOfNodes()));
}

// TIMER_START(distance_table);
std::shared_ptr<std::vector<EdgeWeight>> result_table =
search_engine_ptr->distance_table(phantom_node_vector);
search_engine_ptr->distance_table(phantom_node_target_vector, phantom_node_source_vector);
// TIMER_STOP(distance_table);

if (!result_table)
{
return 400;
}

osrm::json::Array json_array;
const auto number_of_locations = phantom_node_vector.size();
for (const auto row : osrm::irange<std::size_t>(0, number_of_locations))
osrm::json::Array matrix_json_array;
const auto number_of_targets = phantom_node_target_vector.size();
const auto number_of_sources = (phantom_node_source_vector.size()) ? phantom_node_source_vector.size() : number_of_targets;
for (const auto row : osrm::irange<std::size_t>(0, number_of_sources))
{
osrm::json::Array json_row;
auto row_begin_iterator = result_table->begin() + (row * number_of_locations);
auto row_end_iterator = result_table->begin() + ((row + 1) * number_of_locations);
auto row_begin_iterator = result_table->begin() + (row * number_of_targets);
auto row_end_iterator = result_table->begin() + ((row + 1) * number_of_targets);
json_row.values.insert(json_row.values.end(), row_begin_iterator, row_end_iterator);
json_array.values.push_back(json_row);
matrix_json_array.values.push_back(json_row);
}
json_result.values["distance_table"] = matrix_json_array;
if (route_parameters.mapped_points) {
Copy link
Member

Choose a reason for hiding this comment

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

Remove if, since the parameters needs to be dropped. (Code in general is fine.) I would go with source_coordinates and destination_coordinates here.

osrm::json::Array target_coord_json_array;
for (const std::vector<PhantomNode> &phantom_node_vector : phantom_node_target_vector)
{
osrm::json::Array json_coord;
FixedPointCoordinate coord = phantom_node_vector[0].location;
json_coord.values.push_back(coord.lat / COORDINATE_PRECISION);
json_coord.values.push_back(coord.lon / COORDINATE_PRECISION);
target_coord_json_array.values.push_back(json_coord);
}
json_result.values["target_mapped_coordinates"] = target_coord_json_array;
osrm::json::Array source_coord_json_array;
for (const std::vector<PhantomNode> &phantom_node_vector : phantom_node_source_vector)
{
osrm::json::Array json_coord;
FixedPointCoordinate coord = phantom_node_vector[0].location;
json_coord.values.push_back(coord.lat / COORDINATE_PRECISION);
json_coord.values.push_back(coord.lon / COORDINATE_PRECISION);
source_coord_json_array.values.push_back(json_coord);
}
if (source_coord_json_array.values.size())
json_result.values["source_mapped_coordinates"] = source_coord_json_array;
}
json_result.values["distance_table"] = json_array;
// osrm::json::render(reply.content, json_object);
return 200;
}
Expand Down
4 changes: 2 additions & 2 deletions plugins/plugin_base.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ class BasePlugin
virtual const std::string GetDescriptor() const = 0;
virtual int HandleRequest(const RouteParameters &, osrm::json::Object &) = 0;
virtual bool
check_all_coordinates(const std::vector<FixedPointCoordinate> &coordinates) const final
check_all_coordinates(const std::vector<FixedPointCoordinate> &coordinates, const unsigned min = 2) const final
Copy link
Member

Choose a reason for hiding this comment

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

Remove the min check here. Should be handled on caller side.

{
if (2 > coordinates.size() || std::any_of(std::begin(coordinates), std::end(coordinates),
if (min > coordinates.size() || std::any_of(std::begin(coordinates), std::end(coordinates),
[](const FixedPointCoordinate &coordinate)
{
return !coordinate.is_valid();
Expand Down
15 changes: 8 additions & 7 deletions routing_algorithms/many_to_many.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,12 @@ class ManyToManyRouting final
~ManyToManyRouting() {}

std::shared_ptr<std::vector<EdgeWeight>>
operator()(const PhantomNodeArray &phantom_nodes_array) const
operator()(const PhantomNodeArray &phantom_targets_array, const PhantomNodeArray &phantom_sources_array = PhantomNodeArray(0)) const
Copy link
Member

Choose a reason for hiding this comment

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

Don't provide default values here. Parameter order should be const PhantomNodeArray& sources, const PhantomNodeArray& targets.

{
const auto number_of_locations = phantom_nodes_array.size();
const auto number_of_targets = phantom_targets_array.size();
const auto number_of_sources = (phantom_sources_array.size()) ? phantom_sources_array.size() : number_of_targets;
std::shared_ptr<std::vector<EdgeWeight>> result_table =
std::make_shared<std::vector<EdgeWeight>>(number_of_locations * number_of_locations,
std::make_shared<std::vector<EdgeWeight>>(number_of_targets * number_of_sources,
std::numeric_limits<EdgeWeight>::max());

engine_working_data.InitializeOrClearFirstThreadLocalStorage(
Expand All @@ -82,7 +83,7 @@ class ManyToManyRouting final
SearchSpaceWithBuckets search_space_with_buckets;

unsigned target_id = 0;
for (const std::vector<PhantomNode> &phantom_node_vector : phantom_nodes_array)
for (const std::vector<PhantomNode> &phantom_node_vector : phantom_targets_array)
{
query_heap.Clear();
// insert target(s) at distance 0
Expand Down Expand Up @@ -113,7 +114,7 @@ class ManyToManyRouting final

// for each source do forward search
unsigned source_id = 0;
for (const std::vector<PhantomNode> &phantom_node_vector : phantom_nodes_array)
for (const std::vector<PhantomNode> &phantom_node_vector : (phantom_sources_array.size() ? phantom_sources_array : phantom_targets_array))
Copy link
Member

Choose a reason for hiding this comment

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

This should not have a switch. Always use phantom_sources_array. Caller needs to take care to populate both arrays accordingly.

{
query_heap.Clear();
for (const PhantomNode &phantom_node : phantom_node_vector)
Expand All @@ -136,13 +137,13 @@ class ManyToManyRouting final
// explore search space
while (!query_heap.Empty())
{
ForwardRoutingStep(source_id, number_of_locations, query_heap,
ForwardRoutingStep(source_id, number_of_targets, query_heap,
search_space_with_buckets, result_table);
}

++source_id;
}
BOOST_ASSERT(source_id == target_id);
// BOOST_ASSERT(source_id == target_id);
return result_table;
}

Expand Down
Loading