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

Jump to bottom

Refactor matrix_action #4535

Merged
merged 15 commits into from
Jan 31, 2024
Merged

Refactor matrix_action #4535

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
96cb6d6
add a matrix parent class with SourcesToTargets as virtual function, …
nilsnolde Jan 28, 2024
da69b08
Merge branch 'master' into nn-matrix-refactor
nilsnolde Jan 28, 2024
9f3f340
matrix test warning-free
nilsnolde Jan 28, 2024
6919d7c
less includes
nilsnolde Jan 28, 2024
96deff2
Update route_action.cc
nilsnolde Jan 28, 2024
23aa7fb
remove matrix_common.h
nilsnolde Jan 28, 2024
656352d
there was still a expansion_callback in costmatrix.h
nilsnolde Jan 28, 2024
5f93bc5
why tz was updated?..
nilsnolde Jan 28, 2024
e4263ce
changelog
nilsnolde Jan 28, 2024
fefe4d9
remove has_time_ field from costmatrix which is duplicated with matri…
nilsnolde Jan 29, 2024
952bc17
also make Clear() virtual
nilsnolde Jan 29, 2024
c35d876
fix valhalla_run_matrix
nilsnolde Jan 29, 2024
d90c569
another clear() missed in bench
nilsnolde Jan 29, 2024
c5f45f5
Merge branch 'master' into nn-matrix-refactor
nilsnolde Jan 29, 2024
02a3f47
Merge branch 'master' into nn-matrix-refactor
nilsnolde Jan 29, 2024
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
12 changes: 5 additions & 7 deletions src/thor/costmatrix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ class CostMatrix::ReachedMap : public robin_hood::unordered_map<uint64_t, std::v

// Constructor with cost threshold.
CostMatrix::CostMatrix(const boost::property_tree::ptree& config)
: max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_bidir_dijkstras",
: MatrixAlgorithm(config),
max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_bidir_dijkstras",
kInitialEdgeLabelCountBidirDijkstra)),
clear_reserved_memory_(config.get<bool>("clear_reserved_memory", false)),
Copy link
Member Author

Choose a reason for hiding this comment

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

this is now in the base class

max_reserved_locations_count_(
config.get<uint32_t>("max_reserved_locations_costmatrix", kMaxLocationReservation)),
check_reverse_connections_(config.get<bool>("costmatrix_check_reverse_connection", false)),
Expand Down Expand Up @@ -132,19 +132,17 @@ void CostMatrix::SourceToTarget(Api& request,
baldr::GraphReader& graphreader,
const sif::mode_costing_t& mode_costing,
const sif::travel_mode_t mode,
const float max_matrix_distance,
const bool has_time,
const bool invariant,
const ShapeFormat& shape_format) {
Comment on lines -136 to -138
Copy link
Member Author

Choose a reason for hiding this comment

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

I pulled all these out of the signature and set them inside the function instead. was pointless to have them in the signature, it'll all in the request, except for has_time which is being set via the base class's set_has_time()

const float max_matrix_distance) {

LOG_INFO("matrix::CostMatrix");
request.mutable_matrix()->set_algorithm(Matrix::CostMatrix);
bool invariant = request.options().date_time_type() == Options::invariant;
auto shape_format = request.options().shape_format();

// Set the mode and costing
mode_ = mode;
costing_ = mode_costing[static_cast<uint32_t>(mode_)];
access_mode_ = costing_->access_mode();
has_time_ = has_time;

auto& source_location_list = *request.mutable_options()->mutable_sources();
auto& target_location_list = *request.mutable_options()->mutable_targets();
Expand Down
102 changes: 48 additions & 54 deletions src/thor/matrix_action.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,43 +21,9 @@ constexpr uint32_t kCostMatrixThreshold = 5;
namespace valhalla {
namespace thor {

std::string thor_worker_t::matrix(Api& request) {
// time this whole method and save that statistic
auto _ = measure_scope_time(request);

auto& options = *request.mutable_options();
adjust_scores(options);
auto costing = parse_costing(request);

// TODO: do this for others as well
costmatrix_.set_interrupt(interrupt);

// lambdas to do the real work
auto costmatrix = [&](const bool has_time) {
return costmatrix_.SourceToTarget(request, *reader, mode_costing, mode,
max_matrix_distance.find(costing)->second, has_time,
options.date_time_type() == Options::invariant,
options.shape_format());
};
auto timedistancematrix = [&]() {
if (options.shape_format() != no_shape)
add_warning(request, 207);
return time_distance_matrix_.SourceToTarget(request, *reader, mode_costing, mode,
max_matrix_distance.find(costing)->second,
options.matrix_locations(),
options.date_time_type() == Options::invariant);
};
MatrixAlgorithm* thor_worker_t::get_matrix_algorithm(Api& request, const bool has_time) {
Copy link
Member Author

Choose a reason for hiding this comment

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

the diff in here is impossible to read. will show tmrw in code editor what's going on there. the basic gist is:

  • add a get_matrix_algorithm function to thor_worker_t (like for routing), to get the properties of that class
  • remove the lambdas for the matrix algos and decide here which one to use


if (costing == "bikeshare") {
if (options.shape_format() != no_shape)
add_warning(request, 207);
time_distance_bss_matrix_.SourceToTarget(request, *reader, mode_costing, mode,
max_matrix_distance.find(costing)->second,
options.matrix_locations());
return tyr::serializeMatrix(request);
}

Matrix::Algorithm matrix_algo = Matrix::CostMatrix;
Matrix::Algorithm config_algo = Matrix::CostMatrix;
switch (source_to_target_algorithm) {
case SELECT_OPTIMAL:
// TODO - Do further performance testing to pick the best algorithm for the job
Expand All @@ -66,13 +32,13 @@ std::string thor_worker_t::matrix(Api& request) {
case travel_mode_t::kBicycle:
// Use CostMatrix if number of sources and number of targets
// exceeds some threshold
if (static_cast<uint32_t>(options.sources().size()) <= kCostMatrixThreshold ||
static_cast<uint32_t>(options.targets().size()) <= kCostMatrixThreshold) {
matrix_algo = Matrix::TimeDistanceMatrix;
if (static_cast<uint32_t>(request.options().sources().size()) <= kCostMatrixThreshold ||
static_cast<uint32_t>(request.options().targets().size()) <= kCostMatrixThreshold) {
config_algo = Matrix::TimeDistanceMatrix;
}
break;
case travel_mode_t::kPublicTransit:
matrix_algo = Matrix::TimeDistanceMatrix;
config_algo = Matrix::TimeDistanceMatrix;
break;
default:
break;
Expand All @@ -81,32 +47,60 @@ std::string thor_worker_t::matrix(Api& request) {
case COST_MATRIX:
break;
case TIME_DISTANCE_MATRIX:
matrix_algo = Matrix::TimeDistanceMatrix;
config_algo = Matrix::TimeDistanceMatrix;
break;
}

// similar to routing: prefer the exact unidirectional algo if not requested otherwise
// don't use matrix_type, we only need it to set the right warnings for what will be used
bool has_time =
check_matrix_time(request, options.prioritize_bidirectional() ? Matrix::CostMatrix
: Matrix::TimeDistanceMatrix);
if (has_time && !options.prioritize_bidirectional() && source_to_target_algorithm != COST_MATRIX) {
timedistancematrix();
} else if (has_time && options.prioritize_bidirectional() &&
if (has_time && !request.options().prioritize_bidirectional() &&
source_to_target_algorithm != COST_MATRIX) {
return &time_distance_matrix_;
} else if (has_time && request.options().prioritize_bidirectional() &&
source_to_target_algorithm != TIME_DISTANCE_MATRIX) {
costmatrix(has_time);
} else if (matrix_algo == Matrix::CostMatrix) {
// if this happens, the server config only allows for timedist matrix
if (has_time && !options.prioritize_bidirectional()) {
return &costmatrix_;
} else if (config_algo == Matrix::CostMatrix) {
if (has_time && !request.options().prioritize_bidirectional()) {
add_warning(request, 301);
}
costmatrix(has_time);
return &costmatrix_;
} else {
if (has_time && options.prioritize_bidirectional()) {
// if this happens, the server config only allows for timedist matrix
if (has_time && request.options().prioritize_bidirectional()) {
add_warning(request, 300);
}
timedistancematrix();
return &time_distance_matrix_;
}
}

std::string thor_worker_t::matrix(Api& request) {
// time this whole method and save that statistic
auto _ = measure_scope_time(request);

auto& options = *request.mutable_options();
adjust_scores(options);
auto costing = parse_costing(request);

bool has_time =
check_matrix_time(request, options.prioritize_bidirectional() ? Matrix::CostMatrix
: Matrix::TimeDistanceMatrix);

// allow all algos to be cancelled
for (auto* alg : std::vector<MatrixAlgorithm*>{
&costmatrix_,
&time_distance_matrix_,
&time_distance_bss_matrix_,
}) {
alg->set_interrupt(interrupt);
Copy link
Member Author

Choose a reason for hiding this comment

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

I added the interrupt function now for all matrix algos finally

alg->set_has_time(has_time);
}

auto* algo =
costing == "bikeshare" ? &time_distance_bss_matrix_ : get_matrix_algorithm(request, has_time);

algo->SourceToTarget(request, *reader, mode_costing, mode,
max_matrix_distance.find(costing)->second);

return tyr::serializeMatrix(request);
}
} // namespace thor
Expand Down
5 changes: 2 additions & 3 deletions src/thor/optimized_route_action.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ void thor_worker_t::optimized_route(Api& request) {

// Use CostMatrix to find costs from each location to every other location
CostMatrix costmatrix;
costmatrix.set_has_time(check_matrix_time(request, Matrix::CostMatrix));
costmatrix.SourceToTarget(request, *reader, mode_costing, mode,
max_matrix_distance.find(costing)->second,
check_matrix_time(request, Matrix::CostMatrix),
options.date_time_type() == Options::invariant);
max_matrix_distance.find(costing)->second);

// Return an error if any locations are totally unreachable
const auto& correlated =
Expand Down
20 changes: 12 additions & 8 deletions src/thor/timedistancebssmatrix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace thor {

// Constructor with cost threshold.
TimeDistanceBSSMatrix::TimeDistanceBSSMatrix(const boost::property_tree::ptree& config)
: settled_count_(0), current_cost_threshold_(0),
: MatrixAlgorithm(config), settled_count_(0), current_cost_threshold_(0),
max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_dijkstras",
kInitialEdgeLabelCountDijkstras)),
clear_reserved_memory_(config.get<bool>("clear_reserved_memory", false)) {
Expand Down Expand Up @@ -180,10 +180,10 @@ void TimeDistanceBSSMatrix::Expand(GraphReader& graphreader,
template <const ExpansionType expansion_direction, const bool FORWARD>
void TimeDistanceBSSMatrix::ComputeMatrix(Api& request,
baldr::GraphReader& graphreader,
const float max_matrix_distance,
const uint32_t matrix_locations) {
// Run a series of one to many calls and concatenate the results.
const float max_matrix_distance) {
uint32_t matrix_locations = request.options().matrix_locations();

// Run a series of one to many calls and concatenate the results.
auto& origins = FORWARD ? *request.mutable_options()->mutable_sources()
: *request.mutable_options()->mutable_targets();
auto& destinations = FORWARD ? *request.mutable_options()->mutable_targets()
Expand All @@ -210,6 +210,7 @@ void TimeDistanceBSSMatrix::ComputeMatrix(Api& request,
SetOrigin<expansion_direction>(graphreader, origin);
SetDestinationEdges();

uint32_t n = 0;
// Find shortest path
graph_tile_ptr tile;
while (true) {
Expand Down Expand Up @@ -259,6 +260,11 @@ void TimeDistanceBSSMatrix::ComputeMatrix(Api& request,
// Expand forward from the end node of the predecessor edge.
Expand<expansion_direction>(graphreader, pred.endnode(), pred, predindex, false, false,
pred.mode());

// Allow this process to be aborted
if (interrupt_ && (n++ % kInterruptIterationsInterval) == 0) {
(*interrupt_)();
}
}
reset();
}
Expand All @@ -267,13 +273,11 @@ void TimeDistanceBSSMatrix::ComputeMatrix(Api& request,
template void
TimeDistanceBSSMatrix::ComputeMatrix<ExpansionType::forward, true>(Api& request,
baldr::GraphReader& graphreader,
const float max_matrix_distance,
const uint32_t matrix_locations);
const float max_matrix_distance);
template void
TimeDistanceBSSMatrix::ComputeMatrix<ExpansionType::reverse, false>(Api& request,
baldr::GraphReader& graphreader,
const float max_matrix_distance,
const uint32_t matrix_locations);
const float max_matrix_distance);

// Add edges at the origin to the adjacency list
template <const ExpansionType expansion_direction, const bool FORWARD>
Expand Down
42 changes: 13 additions & 29 deletions src/thor/timedistancematrix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,33 +8,14 @@
using namespace valhalla::baldr;
using namespace valhalla::sif;

namespace {
static bool IsTrivial(const uint64_t& edgeid,
const valhalla::Location& origin,
Comment on lines -11 to -13
Copy link
Member Author

Choose a reason for hiding this comment

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

this was duplicated with the exact same function in PathAlgorithm

const valhalla::Location& destination) {
for (const auto& destination_edge : destination.correlation().edges()) {
if (destination_edge.graph_id() == edgeid) {
for (const auto& origin_edge : origin.correlation().edges()) {
if (origin_edge.graph_id() == edgeid &&
origin_edge.percent_along() <= destination_edge.percent_along()) {
return true;
}
}
}
}
return false;
}
} // namespace

namespace valhalla {
namespace thor {

// Constructor with cost threshold.
TimeDistanceMatrix::TimeDistanceMatrix(const boost::property_tree::ptree& config)
: settled_count_(0), current_cost_threshold_(0),
: MatrixAlgorithm(config), settled_count_(0), current_cost_threshold_(0),
max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_dijkstras",
kInitialEdgeLabelCountDijkstras)),
clear_reserved_memory_(config.get<bool>("clear_reserved_memory", false)),
mode_(travel_mode_t::kDrive) {
}

Expand Down Expand Up @@ -194,9 +175,10 @@ void TimeDistanceMatrix::Expand(GraphReader& graphreader,
template <const ExpansionType expansion_direction, const bool FORWARD>
void TimeDistanceMatrix::ComputeMatrix(Api& request,
baldr::GraphReader& graphreader,
const float max_matrix_distance,
const uint32_t matrix_locations,
const bool invariant) {
const float max_matrix_distance) {
bool invariant = request.options().date_time_type() == Options::invariant;
uint32_t matrix_locations = request.options().matrix_locations();

uint32_t bucketsize = costing_->UnitSize();

auto& origins = FORWARD ? *request.mutable_options()->mutable_sources()
Expand Down Expand Up @@ -230,6 +212,7 @@ void TimeDistanceMatrix::ComputeMatrix(Api& request,
SetOrigin<expansion_direction>(graphreader, origin, time_info);
SetDestinationEdges();

uint32_t n = 0;
// Collect edge_ids used for settling a location to determine its time zone
std::unordered_map<uint32_t, baldr::GraphId> dest_edge_ids;
dest_edge_ids.reserve(destinations.size());
Expand Down Expand Up @@ -287,6 +270,11 @@ void TimeDistanceMatrix::ComputeMatrix(Api& request,
// Expand forward from the end node of the predecessor edge.
Expand<expansion_direction>(graphreader, pred.endnode(), pred, predindex, false, time_info,
invariant);

// Allow this process to be aborted
if (interrupt_ && (n++ % kInterruptIterationsInterval) == 0) {
(*interrupt_)();
}
}

reset();
Expand All @@ -308,15 +296,11 @@ void TimeDistanceMatrix::ComputeMatrix(Api& request,
template void
TimeDistanceMatrix::ComputeMatrix<ExpansionType::forward, true>(Api& request,
baldr::GraphReader& graphreader,
const float max_matrix_distance,
const uint32_t matrix_locations,
const bool invariant);
const float max_matrix_distance);
template void
TimeDistanceMatrix::ComputeMatrix<ExpansionType::reverse, false>(Api& request,
baldr::GraphReader& graphreader,
const float max_matrix_distance,
const uint32_t matrix_locations,
const bool invariant);
const float max_matrix_distance);

// Add edges at the origin to the adjacency list
template <const ExpansionType expansion_direction, const bool FORWARD>
Expand Down
1 change: 0 additions & 1 deletion test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ set(tests_with_warnings
logging
maneuversbuilder
mapmatch
matrix
narrativebuilder
node_search
polyline2
Expand Down
Loading
Loading