mark edge segments with interpolated match points #3670

kevinkreiser · 2022-06-30T02:22:58Z

this is an alternative approach to @nilsnolde 's work on #3646

in that pr we essentially called cut segments for every match point, which, while that probably isnt too bad, is technically a worse run time because of the nested looping and allocations as nils pointed out.

in this pr i took a slightly different approach. i refactored cut segments in the following way. previously it would simply split any edge segments where a breakpoint landed as well as the last segment between a pair of states in the hmm. now, while it does that it also marks those segments with the match point index that lies on the given segment even if we arent going to split that segment at the match point (ie even if its not a break). the runtime is unchanged because we still loop over all the segments and all the match points in tandem. the only thing that will cost run time are the two ifs inside of the check where we avoid splitting the segment.

another option would have been to move this logic up further in the call stack to be inside of the MergeRoute function. while this seems like a better place for it (because we would create the segments properly in the first place) this required a lot more surgery because that function doesnt currently loop over the match points. ultimately though this is definitely where the code should be, ill make a comment in the code to that effect

…are interpolated and not breaks

…ter in another loop

…ch_index

kevinkreiser · 2022-06-30T02:25:22Z

src/meili/match_route.cc

      throw std::logic_error("In meili::cutsegments(), unexpectedly unable to locate target edge.");
    }

+    // if its not a break point nor the last segment, then we dont need to split it into two
+    if (!curr_match.is_break_point && curr_idx != last_idx) {


so the main trick is we moved this continue down past where we did the loop to find the segment upon which the current match point lies. this way we have access to it, even if we arent going to cut it below. because i keep the search_segment around for each iteration of the main loop we are gauranteed that the inner loop does no more iterations than it otherwise would have and so this change shouldnt really cost us anything

kevinkreiser · 2022-06-30T02:26:58Z

src/meili/match_route.cc

+      if (search_segment->first_match_idx < 0)
+        search_segment->first_match_idx = curr_idx;
+      if (search_segment->last_match_idx < 0)
+        search_segment->last_match_idx = curr_idx;


i'll admit that i didnt think too hard about how correct this is but rather did the conservative thing, that is only set the index if unset. its possible that i should just always set both of them because they will be corrected below and wont have been set up to this point (ie we can probably remove the ifs) but i didnt want to have to prove that to myself at 10pm 😄

kevinkreiser · 2022-06-30T02:29:02Z

scripts/valhalla_build_timezones

@@ -2,7 +2,7 @@

 error_exit() {
  echo "error: $1" 1>&2
-  exit $(pkg-config geos --modversion | grep -cvF "3.9")
+  #exit $(pkg-config geos --modversion | grep -cvF "3.9")


yeah so we have a bug on geos 3.9*. and in a previous pr we patched it without really checking it. the best thing that we can do here is to not exit if its 3.9. ill change this script to do that. @nilsnolde are you cool if this comes along for the ride? im kind of sick of having to deal with it and have been too lazy to pr it seprately

nilsnolde · 2022-06-30T06:23:21Z

that's great @kevinkreiser , thanks! much easier to understand than my PR. hope we won't have to touch that one for a while now 😄

let me add a bit more extensive test to trace_attributes.

nilsnolde · 2022-06-30T12:04:57Z

huh.. can't set you as a reviewer anymore @kevinkreiser, github weirdo again. anyways, you can let me know if the tests are ok for you like this.

kevinkreiser · 2022-06-30T12:32:36Z

Haha yeah i opened the pr so you have to be reviewer

nilsnolde · 2022-06-30T12:39:18Z

makes sense :D I gratefully approve :)

kevinkreiser · 2022-06-30T13:31:31Z

just had to update the changelog and will merge when the checks are green

ThomasVall · 2023-03-27T09:13:42Z

Hi,

We experienced another edge_index problem even with your fix, we have some values like : "18446744073709551615" with version 3.3.0 of Valhalla.
Our payload contains these :

{
  "shape": [
    {
      "lon": 1.1409,
      "lat": 49.412645
    },
    {
      "lon": 1.140905,
      "lat": 49.41264833333334
    },
    {
      "lon": 1.1409099999999999,
      "lat": 49.41264666666667
    },
    {
      "lon": 1.1409333333333334,
      "lat": 49.412641666666666
    },
    {
      "lon": 1.1409716666666667,
      "lat": 49.412615
    },
    {
      "lon": 1.141005,
      "lat": 49.41258333333333
    }
  ],
  "shape_match": "map_snap",
  "costing": "auto",
  "use_timestamps": false
}

The received response looks like :

{
  "units": "kilometers",
  "osm_changeset": 10754512026,
  "shape": "w_|f}AyjsdA]cAUu@bBaB",
  "confidence_score": 1.0,
  "raw_score": 1.075,
  "admins": [
    {
      "country_code": "FR",
      "country_text": "France"
    }
  ],
  "edges": [
    {
      "truck_route": false,
      "speed_limit": 50,
      "density": 7,
      "sac_scale": 0,
      "shoulder": false,
      "sidewalk": "none",
      "bicycle_network": 0,
      "cycle_lane": "none",
      "lane_count": 1,
      "max_downward_grade": -2,
      "max_upward_grade": 0,
      "weighted_grade": -1.666,
      "mean_elevation": 156,
      "way_id": 133183114,
      "id": 2335160008610,
      "travel_mode": "drive",
      "vehicle_type": "car",
      "surface": "paved_smooth",
      "drive_on_right": true,
      "internal_intersection": false,
      "roundabout": false,
      "bridge": false,
      "tunnel": false,
      "unpaved": false,
      "toll": false,
      "use": "road",
      "traversability": "forward",
      "end_shape_index": 2,
      "begin_shape_index": 0,
      "end_heading": 57,
      "begin_heading": 57,
      "road_class": "residential",
      "speed": 50,
      "length": 0.005,
      "names": [
        "Rue Romain Docquet"
      ],
      "end_node": {
        "intersecting_edges": [
          {
            "walkability": "both",
            "cyclability": "both",
            "driveability": "both",
            "from_edge_name_consistency": false,
            "to_edge_name_consistency": false,
            "begin_heading": 335,
            "use": "road",
            "road_class": "residential"
          }
        ],
        "elapsed_time": 0.38,
        "admin_index": 0,
        "type": "street_intersection",
        "fork": false,
        "time_zone": "Europe/Paris",
        "transition_time": 2.208
      }
    },
    {
      "truck_route": false,
      "density": 7,
      "sac_scale": 0,
      "shoulder": false,
      "sidewalk": "none",
      "bicycle_network": 0,
      "cycle_lane": "none",
      "lane_count": 1,
      "max_downward_grade": 0,
      "max_upward_grade": 3,
      "weighted_grade": 1.666,
      "mean_elevation": 156,
      "way_id": 184783293,
      "id": 894936678306,
      "travel_mode": "drive",
      "vehicle_type": "car",
      "surface": "paved_smooth",
      "drive_on_right": true,
      "internal_intersection": false,
      "roundabout": false,
      "bridge": false,
      "tunnel": false,
      "unpaved": false,
      "toll": false,
      "use": "road",
      "traversability": "both",
      "end_shape_index": 3,
      "begin_shape_index": 2,
      "end_heading": 148,
      "begin_heading": 148,
      "road_class": "residential",
      "speed": 35,
      "length": 0.006,
      "names": [
        "Rue Pierre Tarle"
      ],
      "end_node": {
        "elapsed_time": 3.273,
        "admin_index": 0,
        "type": "street_intersection",
        "fork": false,
        "transition_time": 0.0
      }
    }
  ],
  "matched_points": [
    {
      "lon": 1.140924,
      "lat": 49.41262,
      "type": "matched",
      "edge_index": 0,
      "distance_along_edge": 0.959356,
      "distance_from_trace_point": 3.243988
    },
    {
      "lon": 1.14093,
      "lat": 49.412622,
      "type": "interpolated",
      "edge_index": 0,
      "distance_along_edge": 0.963251,
      "distance_from_trace_point": 3.356317
    },
    {
      "lon": 1.140932,
      "lat": 49.412623,
      "type": "interpolated",
      "edge_index": 18446744073709551615,
      "distance_along_edge": 0.964806,
      "distance_from_trace_point": 3.005524
    },
    {
      "lon": 1.140945,
      "lat": 49.412629,
      "type": "interpolated",
      "edge_index": 18446744073709551615,
      "distance_along_edge": 0.973363,
      "distance_from_trace_point": 1.62578
    },
    {
      "lon": 1.141003,
      "lat": 49.412627,
      "type": "interpolated",
      "edge_index": 1,
      "distance_along_edge": 0.06591,
      "distance_from_trace_point": 2.697996
    },
    {
      "lon": 1.141034,
      "lat": 49.412595,
      "type": "matched",
      "edge_index": 1,
      "distance_along_edge": 0.184857,
      "distance_from_trace_point": 2.532955
    }
  ],
  "alternate_paths": []
}

Did we miss something or is it another bug ?

Regards,
Thomas

nilsnolde · 2023-03-27T10:20:14Z

Yes: #3699

It's smth to do with duplicate waypoints (as in your example, better to sanitize it), but maybe not only. It's part of the bug scrub project, but has very little priority atm. Which can change of course once someone decides to fund a fix:)

ThomasVall · 2023-03-27T11:59:18Z

Thanks @nilsnolde for you quick reply, I edited my example with no duplicate but I still get the same edge_index bug. Is there a workaround for now, or must I still use an older version of Valhalla ?

kevinkreiser · 2023-03-27T13:25:04Z

@ThomasVall regardless of whether this looks like a new but similar bug to some of the others or if its a duplicate. it would be good for you to post this info in the issue so that it doesnt get lost. a reproducible case is always the first step in fixing these things. as @nilsnolde said its hard to find time to work on this bug scrub at the moment but when we do having the test case right there in the issue is very helpful.

thanks for reporting!

nilsnolde and others added 7 commits May 29, 2022 14:28

revert back to old logic with just some slight adjustments

0a71dce

fix test

26b55fa

changelog

31c2269

a refactor to allow modifying segments indices for match points that …

79676ce

…are interpolated and not breaks

do the intuitive thing which is to set the index whenever its not set

faf7868

todo about where we should create the segments and not modify them la…

f01b71e

…ter in another loop

Merge remote-tracking branch 'origin/master' into kk_interpolated_mat…

ef28f74

…ch_index

kevinkreiser requested a review from nilsnolde June 30, 2022 02:23

kevinkreiser commented Jun 30, 2022

View reviewed changes

properly build timezones on geos 3.9*

354888f

nilsnolde force-pushed the kk_interpolated_match_index branch from 9334db9 to 9a8ebe9 Compare June 30, 2022 07:37

remove old test and add more extensive, new test

0524544

nilsnolde force-pushed the kk_interpolated_match_index branch from 9a8ebe9 to 0524544 Compare June 30, 2022 07:42

revert pybind11 update

881af5f

nilsnolde previously approved these changes Jun 30, 2022

View reviewed changes

test also matched points

322ca4a

nilsnolde dismissed their stale review via 322ca4a June 30, 2022 10:53

nilsnolde previously approved these changes Jun 30, 2022

View reviewed changes

kevinkreiser dismissed nilsnolde’s stale review via cea174c June 30, 2022 13:30

Update CHANGELOG.md

cea174c

nilsnolde approved these changes Jun 30, 2022

View reviewed changes

nilsnolde mentioned this pull request Jun 30, 2022

Respect interpolated matches when building matched route #3646

Closed

kevinkreiser merged commit 67f120a into master Jun 30, 2022

kevinkreiser deleted the kk_interpolated_match_index branch June 30, 2022 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mark edge segments with interpolated match points #3670

mark edge segments with interpolated match points #3670

kevinkreiser commented Jun 30, 2022 •

edited

kevinkreiser Jun 30, 2022

kevinkreiser Jun 30, 2022

kevinkreiser Jun 30, 2022

nilsnolde Jun 30, 2022

nilsnolde commented Jun 30, 2022 •

edited

nilsnolde commented Jun 30, 2022

kevinkreiser commented Jun 30, 2022

nilsnolde commented Jun 30, 2022

kevinkreiser commented Jun 30, 2022

ThomasVall commented Mar 27, 2023 •

edited

nilsnolde commented Mar 27, 2023

ThomasVall commented Mar 27, 2023

kevinkreiser commented Mar 27, 2023

mark edge segments with interpolated match points #3670

mark edge segments with interpolated match points #3670

Conversation

kevinkreiser commented Jun 30, 2022 • edited

kevinkreiser Jun 30, 2022

Choose a reason for hiding this comment

kevinkreiser Jun 30, 2022

Choose a reason for hiding this comment

kevinkreiser Jun 30, 2022

Choose a reason for hiding this comment

nilsnolde Jun 30, 2022

Choose a reason for hiding this comment

nilsnolde commented Jun 30, 2022 • edited

nilsnolde commented Jun 30, 2022

kevinkreiser commented Jun 30, 2022

nilsnolde commented Jun 30, 2022

kevinkreiser commented Jun 30, 2022

ThomasVall commented Mar 27, 2023 • edited

nilsnolde commented Mar 27, 2023

ThomasVall commented Mar 27, 2023

kevinkreiser commented Mar 27, 2023

kevinkreiser commented Jun 30, 2022 •

edited

nilsnolde commented Jun 30, 2022 •

edited

ThomasVall commented Mar 27, 2023 •

edited