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.

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

timeinfo tracking in map matching #2712

Merged
merged 16 commits into from
Jan 29, 2021
Merged

timeinfo tracking in map matching #2712

merged 16 commits into from
Jan 29, 2021

Conversation

kevinkreiser
Copy link
Member

Currently map matching (either fast or robust versions) dont use the TimeInfo object to track time along the route (once the match is found). This pr adds that tracking into both versions. It also makes some changes in the fast map matcher to prepare for supporting multileg matches. Right now i've gone a little too far with the multi leg support that sometimes the edge walk fails. So i'll either need to finish support for that or rollback some of the changes.

@kevinkreiser kevinkreiser marked this pull request as ready for review January 23, 2021 16:47
Comment on lines -12 to -20
namespace {
PointLL to_ll(const valhalla::Location& l) {
return PointLL{l.ll().lng(), l.ll().lat()};
}
void from_ll(valhalla::Location* l, const PointLL& p) {
l->mutable_ll()->set_lat(p.lat());
l->mutable_ll()->set_lng(p.lng());
}
} // namespace
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 existed in the header above so i did this little clean up

uint32_t origin_epoch = compute_origin_epoch(edge_segments, matcher, options);
std::string date_time = options.shape(0).has_date_time() ? options.shape(0).date_time() : "";
valhalla::baldr::DateTime::tz_sys_info_cache_t tz_cache;
auto time_info = init_time_info(edge_segments, matcher, options, &tz_cache);
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 the main point of this pr. we dont manually track second of week or the timezone etc. we do it with the time_info object as all the other time based algorithms do it

}
}
valhalla::baldr::DateTime::tz_sys_info_cache_t tz_cache;
auto time_info = init_time_info(reader, options, &tz_cache);
Copy link
Member Author

Choose a reason for hiding this comment

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

so for the shape walking map matcher (ie the faster less robust one) we also switch to using time_info

valhalla::Options& options,
std::vector<PathInfo>& path_infos) {
std::vector<std::vector<PathInfo>>& legs) {
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 started refactoring this method to support multileg routes but there needs to be a little bit more refactoring in the guts of these algorithm to make it possible. i started trying it but it was getting too hair so i aborted that for now

*options.mutable_locations()->rbegin(),
std::list<valhalla::Location>{}, leg, {"edge_walk"}, interrupt);
} else {
std::vector<std::vector<PathInfo>> legs;
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah so i refactored this so that when we finally get multileg support in the fast map matching algorithm this code will already be done, but as it stands today its still not supported

genadz
genadz previously approved these changes Jan 27, 2021
@gknisely
Copy link
Member

@kevinkreiser do we have existing tests already or do we need some

@kevinkreiser
Copy link
Member Author

@gknisely there are existing ones that cover depart_at but time invariant doesnt exist so far as i know. that behavior though is all wrapped into the timeinfo object. do you want me to add a few simple test cases just in case. should be quick

genadz
genadz previously approved these changes Jan 27, 2021
@gknisely
Copy link
Member

@gknisely there are existing ones that cover depart_at but time invariant doesnt exist so far as i know. that behavior though is all wrapped into the timeinfo object. do you want me to add a few simple test cases just in case. should be quick

@kevinkreiser yes. just to be safe

@@ -44,17 +44,17 @@ CandidateCollector::WithinSquaredDistance(const midgard::PointLL& location,
continue;
}

// Get the edge and its opposing edge. Transition edges are not
// allowed so we do not need to check node levels.
const auto opp_edgeid = reader_.GetOpposingEdgeId(edgeid, tile);
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 a bug. when you get the opposing id with the tile of the regular edge it can flip modify the tile to be the wrong tile which we use again to refer to the original edge. the code was throwing an exception, getting caught and failing the map match. this would happen on all map match input points which have radii large enough to intersect an edge that crosses a tile boundary

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -1021,11 +1021,8 @@ void GraphTileBuilder::AddBins(const std::string& tile_dir,

// Add a predicted speed profile for a directed edge.
void GraphTileBuilder::AddPredictedSpeed(const uint32_t idx,
const std::vector<int16_t>& profile,
const std::array<int16_t, kCoefficientCount>& coefficients,
Copy link
Member Author

Choose a reason for hiding this comment

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

it was stupid to take a vector when we wanted the size of the thing to be exactly a certain length. std::array does this. so lets use that instead

set_target_properties(${TESTNAME} PROPERTIES FOLDER "Tests")
target_compile_definitions(${TESTNAME} PRIVATE
VALHALLA_SOURCE_DIR="${VALHALLA_SOURCE_DIR}/"
VALHALLA_BUILD_DIR="${VALHALLA_BUILD_DIR}/")
Copy link
Member Author

Choose a reason for hiding this comment

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

need this to reference properly the timezone sqlite that is built for use in timedependent tests

@@ -247,7 +247,7 @@ TEST(AutoDataFix, deprecation) {
class AlgorithmTest : public ::testing::Test {
protected:
static gurka::map map;
static uint32_t current, historical, constrained;
static uint32_t current, historical, constrained, freeflow;
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 used this test as a model for what i did in the map matching test. they are basically idential and return identical results

@@ -84,7 +84,7 @@ using LiveTrafficCustomize = std::function<void(valhalla::baldr::GraphReader&,
void customize_live_traffic_data(const boost::property_tree::ptree& config,
const LiveTrafficCustomize& setter_cb);

using HistoricalTrafficCustomize = std::function<std::vector<int16_t>(DirectedEdge&)>;
using HistoricalTrafficCustomize = std::function<std::array<float, kBucketsPerWeek>(DirectedEdge&)>;
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 way more useful if you give it the raw speeds and let it convert it to the format stored in the tiles

}

// map matching currently only allows forward time tracking and invariant so types 0, 1, 3.
// type 2 arrive_by is not yet supported
Copy link
Member Author

Choose a reason for hiding this comment

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

we check all the different types of su pported timedependent map matching as well as all the different sources of speed data

@gknisely gknisely self-requested a review January 29, 2021 13:52
@kevinkreiser kevinkreiser merged commit ce39088 into master Jan 29, 2021
@kevinkreiser kevinkreiser deleted the kk_timedep_mapmatch branch January 5, 2022 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants