-
Notifications
You must be signed in to change notification settings - Fork 673
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
Summary Doesnt Match Accumulated Maneuvers #2195
Conversation
R"({"locations":[{"lat":52.048267,"lon":5.074825},{"lat":52.114622,"lon":5.131816}],"costing":"auto"})"; | ||
auto response = tester.test(request); | ||
|
||
// loop over all routes all legs |
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.
first part of the test is to loop over the internal protobuf route representations, both the path and the directions objects and compare that they all add up to the same thing
++trip_route; | ||
} | ||
|
||
// get the json |
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.
then we loop over valhalla flavored json and we have to give 10ms of tolerance because in the end we do rounding when we serialize the values to json
EXPECT_NEAR(accumulated_time, leg["summary"].GetObject()["time"].GetDouble(), .01); | ||
} | ||
|
||
// get the osrm json |
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.
finally we loop over the osrm json to check that that also adds up properly
route->emplace("duration", json::fp_t{duration, 1}); | ||
distance *= imperial ? 1609.344 : 1000.0; | ||
route->emplace("distance", json::fp_t{distance, 3}); | ||
route->emplace("duration", json::fp_t{duration, 3}); |
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.
are milliseconds too fine a measure?
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.
👍 milliseconds makes sense to me to minimize accumulated error
@@ -17,6 +17,7 @@ config = { | |||
'user_agent': optional(str), | |||
'tile_url': optional(str), | |||
'tile_url_gz': optional(bool), | |||
'concurrency': optional(int), |
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.
unrelated change, but i realized it was an undocumented feature in tile building, so i added it
@@ -557,7 +557,7 @@ class EnhancedTripLeg_Node { | |||
return mutable_node_->type(); | |||
} | |||
|
|||
uint32_t elapsed_time() const { | |||
double elapsed_time() const { | |||
return mutable_node_->elapsed_time(); |
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.
technically didnt need to do this as its only for debug output but, it didnt feel right to truncate the number
@@ -142,7 +142,7 @@ message DirectionsLeg { | |||
optional string text_instruction = 2; // text instruction | |||
repeated StreetName street_name = 3; // street names | |||
optional float length = 4; // kilometers or miles based on units |
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.
should length
be switched to double
too?
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.
i thought about it but it seemed somewhat unrelated to the PR at hand. i could expand the pr to test lengths as well. is that the concensus?
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.
I think length can be handled in a separate PR. I was just a bit confused as I saw spots in the code where you change the precision of distance
in the response.
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.
i was thinking to be consistent since distance
was returned as double
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.
i changed the decimal places in json but not the internal precision (which is float). we should change internal distance to be double but at the moment that is a larger change
well oops. i didnt realize that other tests were relying on this (even though i wrote them). |
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.
if tests pass then 🚢
all builds pass. going to update changelog and merge |
* master: Clarify cmake flag preconditions. Fix coverage flag. Include appropriate license header. Disable -Werror in CI build. Remove -Wconversion Update README and CI configuration. Revert coverage flags back to being PUBLIC. Remove stray comment. Refactored flags to use a function; fixed valhalla_module target; flag for Werror Adds a cmake module for libcxx flag macros. Stop impact, PH, turn channel speeds, and tagged speed updates (#2198) add isochrone snapped center coordinates to response (#2111) Summary Doesnt Match Accumulated Maneuvers (#2195)
distance *= imperial ? 1609.34f : 1000.0f; | ||
route->emplace("distance", json::fp_t{distance, 1}); | ||
route->emplace("duration", json::fp_t{duration, 1}); | ||
distance *= imperial ? 1609.344 : 1000.0; |
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.
Can you factor this out into a couple of constants somewhere? Do these already exist somewhere that could be reused?
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.
there are distance constants in midgard/constants.h.
constexpr float kKmPerMile = 1.60934f;
Just need to add kMetersPerMile
as the title says, the summary time information doesnt match the time you accumulate from the maneuver object. even for relatively short routes we seem to be accumulating rounding error in the summary or truncating on the maneuvers
so it turns out that we do a bunch of truncation in odin when we create the directions object. so ive updated all of those to double precision. additionally i added 3 decimal places of precision to the output in the json. what do people think of that. millisecond resolution. perhaps we only need 1 decimal place but the trick is that if you do that you accumulate error faster where you have a lot of maneuvers.