Isochrone: handle origin edges properly #2990

merkispavel · 2021-04-09T14:21:11Z

Issue

The main cause of the issue is a long origin edge. See the diff for more details

Added a fix to handle origin edges properly(see Isochrone::UpdateIsoTile method)
Added a fix to handle long edges properly(see Isochrone::ShouldExpand method)). fixes Isochrone: isochrone visits edges incorrectly #2973

Inputs: planet.tar.zip
Request: curl http://localhost:8002/isochrone --data '{"locations":[{"lat":39.803886,"lon":116.723650}],"costing":"pedestrian","contours":[{"time":10}]}'

Before	After

Before	After

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

merkispavel · 2021-04-09T14:23:48Z

src/thor/isochrone.cc

@@ -200,76 +245,59 @@ void Isochrone::UpdateIsoTile(const EdgeLabel& pred,
  }

  // Get the time and distance at the end node of the predecessor
-  // TODO - do we need partial shape from origin location to end of edge?


It's your the hour of glory, 4-years-old-TODO

merkispavel · 2021-04-09T14:24:58Z

src/thor/isochrone.cc

+      pred.predecessor() == kInvalidLabel ? 0.f : bdedgelabels_[pred.predecessor()].path_distance();
+  // prune the edge if its start is above max contour
+  if (time > max_seconds_ && distance > max_meters_)
+    return ExpansionRecommendation::prune_expansion;


I wasn't sure if this PR is a good place for this commit or I should create standalone one. However, both fixes are need for the illustrated issue(i.e long-origin-edge-issue)

issue #2973

Ah yes, that does explain the issue! Thanks 👍

mandeepsandhu · 2021-04-09T14:36:04Z

@merkispavel what about long edges causes the issue? from some the before pics I can see that the the isochrone does not expand to certain road segments. Whats the root-cause for that?

merkispavel · 2021-04-09T14:41:23Z

@mandeepsandhu My bad, I haven't said anything about that.
There's an example in #2973. Let me know if it makes long-edge-issue clearer

src/thor/isochrone.cc

mandeepsandhu · 2021-04-09T17:20:01Z

test/isochrone.cc

@@ -100,23 +100,23 @@ TEST(Isochrones, Basic) {

  {
    const auto expected =
-        R"({"features":[{"properties":{"fill-opacity":0.33,"fill":"#bf4040","fillColor":"#bf4040","color":"#bf4040","fillOpacity":0.33,"opacity":0.33,"contour":9,"metric":"time"},"geometry":{"coordinates":[[5.040321,52.125973],[5.033321,52.118968],[5.023263,52.123937],[5.026321,52.120697],[5.031766,52.118382],[5.029321,52.113584],[5.034321,52.116540],[5.049421,52.111037],[5.049845,52.106937],[5.053206,52.103822],[5.058321,52.102772],[5.060634,52.103937],[5.060840,52.098937],[5.057224,52.098937],[5.059007,52.098623],[5.057751,52.096937],[5.060272,52.094888],[5.062321,52.096483],[5.063768,52.093937],[5.063321,52.092391],[5.058701,52.092937],[5.054983,52.088275],[5.048321,52.086875],[5.047321,52.087983],[5.045321,52.085858],[5.048774,52.085390],[5.049321,52.083138],[5.049794,52.085464],[5.051766,52.085382],[5.050909,52.083937],[5.054201,52.081937],[5.071321,52.081586],[5.071556,52.076937],[5.069321,52.075623],[5.061002,52.077256],[5.059287,52.073903],[5.062321,52.076429],[5.066321,52.075319],[5.067613,52.070937],[5.060321,52.069593],[5.054919,52.070937],[5.060329,52.078937],[5.059321,52.081147],[5.057797,52.078461],[5.054993,52.077937],[5.055660,52.074598],[5.052834,52.074450],[5.053321,52.077731],[5.050321,52.072423],[5.051236,52.075937],[5.047707,52.078323],[5.048321,52.081425],[5.043259,52.075937],[5.042771,52.072487],[5.035321,52.073258],[5.034141,52.077937],[5.029369,52.075937],[5.030321,52.074810],[5.022061,52.075197],[5.021982,52.073598],[5.025321,52.073406],[5.026952,52.071568],[5.025209,52.067937],[5.029839,52.071419],[5.033321,52.071573],[5.054321,52.068238],[5.056321,52.065249],[5.059321,52.067220],[5.063321,52.066206],[5.064688,52.063304],[5.067613,52.061937],[5.065639,52.057937],[5.067760,52.057376],[5.068003,52.054937],[5.069968,52.058290],[5.073608,52.058224],[5.071012,52.057246],[5.069750,52.051508],[5.066428,52.052044],[5.066186,52.051072],[5.072978,52.046594],[5.089321,52.046308],[5.090321,52.047264],[5.093321,52.046314],[5.094321,52.048197],[5.098321,52.046251],[5.101182,52.051076],[5.104099,52.050159],[5.107570,52.051186],[5.107870,52.048486],[5.111321,52.048213],[5.112321,52.046219],[5.113321,52.049186],[5.114893,52.046509],[5.118321,52.046164],[5.120321,52.049163],[5.124321,52.048191],[5.141321,52.050285],[5.142321,52.048257],[5.145071,52.052187],[5.155792,52.051466],[5.156139,52.054119],[5.159029,52.054937],[5.155145,52.055761],[5.155079,52.057179],[5.159927,52.058937],[5.157321,52.059556],[5.153088,52.057704],[5.151701,52.063937],[5.149182,52.063798],[5.149126,52.065937],[5.152321,52.068117],[5.156739,52.064355],[5.159112,52.064937],[5.158932,52.066548],[5.151959,52.070937],[5.161802,52.079456],[5.163074,52.091937],[5.159648,52.093264],[5.153648,52.102937],[5.161819,52.104937],[5.154033,52.104649],[5.148684,52.113299],[5.149527,52.117937],[5.146321,52.122251],[5.144825,52.119937],[5.147974,52.113284],[5.142321,52.110977],[5.143963,52.108937],[5.141321,52.106324],[5.140321,52.108764],[5.133321,52.105352],[5.134211,52.106937],[5.131321,52.107348],[5.129321,52.104532],[5.115321,52.106504],[5.111629,52.105937],[5.111321,52.104524],[5.107321,52.109954],[5.103321,52.106677],[5.100321,52.107727],[5.097321,52.105768],[5.098469,52.109085],[5.094321,52.107836],[5.092321,52.110126],[5.080321,52.112600],[5.081321,52.107534],[5.084321,52.106528],[5.087321,52.107447],[5.089651,52.105607],[5.085321,52.103591],[5.082407,52.106023],[5.077321,52.106618],[5.078969,52.103937],[5.077558,52.102937],[5.080371,52.099937],[5.078321,52.100645],[5.077321,52.098424],[5.071321,52.098791],[5.070321,52.097363],[5.055535,52.110937],[5.059321,52.112586],[5.063234,52.110024],[5.057778,52.115937],[5.058321,52.117587],[5.062826,52.117937],[5.062321,52.119025],[5.057536,52.119937],[5.058321,52.118287],[5.055928,52.119544],[5.047308,52.116950],[5.049367,52.114983],[5.055683,52.115299],[5.053596,52.111662],[5.051321,52.111620],[5.038321,52.116250],[5.035837,52.118453],[5.037321,52.121383],[5.039321,52.119998],[5.041321,52.120520],[5.038528,52.122144],[5.040321,52.125973]],"type":"LineString"},"type":"Feature"}],"type":"FeatureCollection"})";
+        R"({"features":[{"properties":{"fill-opacity":0.33,"fill":"#bf4040","fillColor":"#bf4040","color":"#bf4040","fillOpacity":0.33,"opacity":0.33,"contour":9,"metric":"time"},"geometry":{"coordinates":[[5.040321,52.125973],[5.033321,52.118968],[5.023263,52.123937],[5.026321,52.120697],[5.031766,52.118382],[5.029321,52.113584],[5.034321,52.116540],[5.049421,52.111037],[5.049845,52.106937],[5.053206,52.103822],[5.058321,52.102772],[5.060634,52.103937],[5.060840,52.098937],[5.057224,52.098937],[5.059007,52.098623],[5.057751,52.096937],[5.060272,52.094888],[5.062321,52.096483],[5.063768,52.093937],[5.063321,52.092391],[5.058701,52.092937],[5.054983,52.088275],[5.048321,52.086875],[5.047321,52.087983],[5.045321,52.085858],[5.048774,52.085390],[5.049321,52.083138],[5.049794,52.085464],[5.051766,52.085382],[5.050909,52.083937],[5.054201,52.081937],[5.071321,52.081586],[5.071556,52.076937],[5.069321,52.075623],[5.061002,52.077256],[5.059287,52.073903],[5.062321,52.076429],[5.066321,52.075319],[5.067613,52.070937],[5.060321,52.069593],[5.054919,52.070937],[5.060329,52.078937],[5.059321,52.081147],[5.057797,52.078461],[5.054993,52.077937],[5.055660,52.074598],[5.052834,52.074450],[5.053321,52.077731],[5.050321,52.072423],[5.051236,52.075937],[5.047707,52.078323],[5.048321,52.081425],[5.043259,52.075937],[5.042771,52.072487],[5.035321,52.073258],[5.034141,52.077937],[5.029369,52.075937],[5.030321,52.074810],[5.022061,52.075197],[5.021982,52.073598],[5.025321,52.073406],[5.026952,52.071568],[5.025209,52.067937],[5.029839,52.071419],[5.033321,52.071573],[5.054321,52.068238],[5.056321,52.065249],[5.059321,52.067220],[5.063321,52.066206],[5.064688,52.063304],[5.067613,52.061937],[5.065639,52.057937],[5.067760,52.057376],[5.068003,52.054937],[5.069968,52.058290],[5.073608,52.058224],[5.071012,52.057246],[5.069750,52.051508],[5.066428,52.052044],[5.066186,52.051072],[5.072978,52.046594],[5.089321,52.046308],[5.090321,52.047264],[5.093321,52.046314],[5.094321,52.048197],[5.098321,52.046251],[5.101182,52.051076],[5.104099,52.050159],[5.107570,52.051186],[5.107870,52.048486],[5.111321,52.048213],[5.112321,52.046219],[5.113321,52.049186],[5.114893,52.046509],[5.118321,52.046164],[5.120321,52.049163],[5.124321,52.048191],[5.141321,52.050285],[5.142321,52.048257],[5.145071,52.052187],[5.155792,52.051466],[5.156139,52.054119],[5.159029,52.054937],[5.155145,52.055761],[5.155079,52.057179],[5.159927,52.058937],[5.152162,52.058778],[5.151688,52.063937],[5.149182,52.063798],[5.149126,52.065937],[5.152321,52.068117],[5.157321,52.064146],[5.159112,52.064937],[5.158932,52.066548],[5.151959,52.070937],[5.161802,52.079456],[5.163074,52.091937],[5.159648,52.093264],[5.153648,52.102937],[5.161819,52.104937],[5.154033,52.104649],[5.148684,52.113299],[5.149527,52.117937],[5.146321,52.122251],[5.144825,52.119937],[5.147974,52.113284],[5.142321,52.110977],[5.143963,52.108937],[5.141321,52.106324],[5.140321,52.108764],[5.133321,52.105352],[5.134211,52.106937],[5.131321,52.107348],[5.129321,52.104532],[5.115321,52.106504],[5.111629,52.105937],[5.111321,52.104524],[5.107321,52.109954],[5.103321,52.106677],[5.100321,52.107727],[5.097321,52.105768],[5.098469,52.109085],[5.094321,52.107836],[5.092321,52.110126],[5.080321,52.112600],[5.081321,52.107534],[5.084321,52.106528],[5.087321,52.107447],[5.089651,52.105607],[5.085321,52.103591],[5.082407,52.106023],[5.077321,52.106618],[5.078969,52.103937],[5.077558,52.102937],[5.080371,52.099937],[5.078321,52.100645],[5.077321,52.098424],[5.071321,52.098791],[5.070321,52.097363],[5.055535,52.110937],[5.059321,52.112586],[5.063234,52.110024],[5.057778,52.115937],[5.058321,52.117587],[5.062826,52.117937],[5.062321,52.119025],[5.057536,52.119937],[5.058321,52.118287],[5.055928,52.119544],[5.047308,52.116950],[5.049367,52.114983],[5.055683,52.115299],[5.053596,52.111662],[5.051321,52.111620],[5.038321,52.116250],[5.035837,52.118453],[5.037321,52.121383],[5.039321,52.119998],[5.041321,52.120520],[5.038528,52.122144],[5.040321,52.125973]],"type":"LineString"},"type":"Feature"}],"type":"FeatureCollection"})";
    const auto request =
        R"({"locations":[{"lat":52.078937,"lon":5.115321}],"costing":"auto","contours":[{"time":9}],"polygons":false,"denoise":0.2,"generalize":100})";


I see we've updated the tests. Are these tests exercising the "long edge" issue? If yes, then 👍 , if no, then maybe add a test (gurka or otherwise)?

Gurka testing it is a good idea. I'll do it 👍

@mandeepsandhu gurka tests are added

merkispavel · 2021-04-12T22:09:30Z

PR is ready for the second round of review. Feel free to merge it if it looks good

kevinkreiser

looks good to me!

merkispavel added 3 commits April 9, 2021 15:19

Isochrone: process origin edges properly

5317949

Isochrone: prune edge if its start is above max contour

3a10b95

Update CHANGELOG.md

209c8b3

merkispavel commented Apr 9, 2021

View reviewed changes

merkispavel requested a review from dnesbitt61 April 9, 2021 14:25

merkispavel mentioned this pull request Apr 9, 2021

Isochrone: isochrone visits edges incorrectly #2973

Closed

kevinkreiser reviewed Apr 9, 2021

View reviewed changes

src/thor/isochrone.cc Show resolved Hide resolved

kevinkreiser reviewed Apr 9, 2021

View reviewed changes

src/thor/isochrone.cc Outdated Show resolved Hide resolved

kevinkreiser reviewed Apr 9, 2021

View reviewed changes

src/thor/isochrone.cc Show resolved Hide resolved

merkispavel added 3 commits April 9, 2021 19:02

Fix: resample first

eecd315

Fix tests

dd351ed

Bugfix: use full edge length for resampling

0128ae0

mandeepsandhu reviewed Apr 9, 2021

View reviewed changes

merkispavel added 3 commits April 12, 2021 18:11

Merge remote-tracking branch 'origin/master' into isochrone-origin-edges

a7a5c60

Add isochrone action to gurka

0168cc3

Add isochrone tests for origin and long edges

d68e0c6

merkispavel requested review from mandeepsandhu and kevinkreiser April 12, 2021 22:08

kevinkreiser approved these changes Apr 12, 2021

View reviewed changes

merkispavel merged commit 3419fd2 into master Apr 12, 2021

merkispavel deleted the isochrone-origin-edges branch April 12, 2021 23:17

dnesbitt61 mentioned this pull request Apr 16, 2021

Performance Drop in Long Bicycle Route #3007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isochrone: handle origin edges properly #2990

Isochrone: handle origin edges properly #2990

merkispavel commented Apr 9, 2021 •

edited

Loading

merkispavel Apr 9, 2021

merkispavel Apr 9, 2021 •

edited

Loading

merkispavel Apr 9, 2021 •

edited

Loading

mandeepsandhu Apr 9, 2021

mandeepsandhu commented Apr 9, 2021

merkispavel commented Apr 9, 2021

mandeepsandhu Apr 9, 2021

merkispavel Apr 9, 2021

merkispavel Apr 12, 2021

merkispavel commented Apr 12, 2021

kevinkreiser left a comment

Isochrone: handle origin edges properly #2990

Isochrone: handle origin edges properly #2990

Conversation

merkispavel commented Apr 9, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

merkispavel Apr 9, 2021

Choose a reason for hiding this comment

merkispavel Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

merkispavel Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

mandeepsandhu Apr 9, 2021

Choose a reason for hiding this comment

mandeepsandhu commented Apr 9, 2021

merkispavel commented Apr 9, 2021

mandeepsandhu Apr 9, 2021

Choose a reason for hiding this comment

merkispavel Apr 9, 2021

Choose a reason for hiding this comment

merkispavel Apr 12, 2021

Choose a reason for hiding this comment

merkispavel commented Apr 12, 2021

kevinkreiser left a comment

Choose a reason for hiding this comment

merkispavel commented Apr 9, 2021 •

edited

Loading

merkispavel Apr 9, 2021 •

edited

Loading

merkispavel Apr 9, 2021 •

edited

Loading