Car loops caused by turn restrictions by leonardehrenfried · Pull Request #3399 · opentripplanner/OpenTripPlanner

leonardehrenfried · 2021-04-01T14:15:25Z

Summary

This problem has been discussed in the OTP2 dev meeting and is described in #3393.

Short recap: (Car) routes with required loops (example) lead to an error.

Our solution is to allow those loops close to the start or end of the route by adding a check in the dominance function. This is the actual change of the PR: 88f28d0

It also updates Junit to the latest version.

Alternative solutions

Both @flaktack and @gmellemstrand have made suggestions how the problem could be solved:

you could simply allow loops anywhere (but that will increase the search space)
you could allow only a certain length of loop, say 1 kilometer. however, it doesn't seem like the distance driven is currently tracked in State so that would need to be added.

Lets discuss in the next dev meeting.

Issue

Fixes #3393
Fixes #3402

Unit tests

The bulk of the PR actually consists of unit tests. It introduces a technique not used a lot in current OTP tests: It builds a graph from actual OSM data, runs queries on it and uses the resulting polyline for assertions.

For tricky situations like the one we are discussing here I find it hard to reproduce the issues by building up individual edges manually and therefore opted for this approach. It also helps to build up a corpus of routes that make it easier to tell if an algorithm change has unintended side effects.

The downside is that building the graph takes longer than the ordinary test setup: building the graph for a small city like Herrenberg takes around 3-4 seconds. We should discuss if this is too high a price to pay or if restructuring these tests (perhaps by running them in a separate integration test phase) is worth doing.

In order to achieve this I introduced an interface for the graph sources so that it's easier to specify the exact OSM and GTFS files in a test graph.

This PR also introduces a new helper tool to give you a nice error message when two polylines are not the same. It links you to a website where you can see the lines visualised on a map.

Code style

✅

Documentation

✅

Changelog

✅

leonardehrenfried · 2021-04-01T14:53:14Z

        }
    }

-    public static Graph buildGraph(String path) {


This code was unused.

leonardehrenfried · 2021-04-01T14:53:22Z

        return instance;
    }

-    public GtfsContext getPortlandContext() {


This code was unused.

leonardehrenfried · 2021-04-06T13:15:10Z

After our conversation today at the OTP2 meeting I reworked this PR a little and have some results so share.

Test graph building speed

I reduced the size of the test graph by both using a smaller area and stripping out anything that is not a road. Building the graph is now reported to take 0 seconds, so presumably it's only a few hundred milliseconds. I would say @t2gran's concerns are addressed.

Performance impact

The good news is that I could generate a trip from Oslo to Trondheim which is a 480km trip.

Sadly the performance hit is quite significant: before the change it would take 2.5 seconds to calculate and after removing the turn restrictions check it takes around 4 seconds. 😿

I'm not sure if this is an acceptable slowdown.

leonardehrenfried · 2021-04-06T20:15:24Z

I've taken a more scientific approach and benchmarked the various implementations more thoroughly.

To do this, I've used apache bench (ab) to run first 100 warm-up requests and then 10 requests to do the actual measuring.

Commands

Warm up

ab -n 100 -c 5 "http://localhost:8080/otp/routers/default/plan?fromPlace=59.9295600665038%2C10.767974853515625&toPlace=63.43331707559086%2C10.4205322265625&time=3%3A03pm&date=04-06-2021&mode=CAR&arriveBy=false&wheelchair=false&debugItineraryFilter=false&locale=en"

Measuring

ab -n 10 "http://localhost:8080/otp/routers/default/plan?fromPlace=59.9295600665038%2C10.767974853515625&toPlace=63.43331707559086%2C10.4205322265625&time=3%3A03pm&date=04-06-2021&mode=CAR&arriveBy=false&wheelchair=false&debugItineraryFilter=false&locale=en"

Results

No loops allowed (current `dev-2.x`)

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  2168 2451 292.3   2358    2939
Waiting:     2167 2449 292.2   2357    2937
Total:       2168 2451 292.3   2358    2939

Loops are always allowed

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       0
Processing:  3395 4344 566.4   4494    5026
Waiting:     3392 4337 572.1   4489    5026
Total:       3395 4344 566.4   4494    5026

Only at start or end

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  2001 2530 806.6   2349    4756
Waiting:     2000 2529 806.7   2348    4755
Total:       2001 2530 806.7   2349    4756

Lets discuss these numbers in the next dev meeting.

gmellemstrand · 2021-04-09T09:38:52Z

+         * The following variable determines how close you have to be to the start or the end point for it to be
+         * considered "close enough for a loop".
+         */
+        if (a.backEdge != b.getBackEdge()


Should there be a check for car mode here?

Well, technically turn restrictions also apply to cycling, however you can always get off and push the bike. Maybe this is a good idea.

So I checked my test case and if I only allow the loop for cars the bike will take a route like this: https://leonard.io/polyline-visualiser/?base64=b3VxZ0h9bWN1QGdBRV1VfUJhQV1RfUBddUFzQFtTQW1ARWVAQVVHQkFIS0Y_SFNIR3hA

That's ok, I guess.

I see your point that it may be relevant to biking too. I will leave it up to you whether to include biking or. not, as you have worked more on bike routing than me

gmellemstrand · 2021-04-09T09:41:21Z

I discussed this a little bit with @flaktack in the chat window of the last OTP meeting. I think for one-to-many searches we would need to check for loop edges for the entire search, as we do not know if we are close to one of the acceptable destinations.

leonardehrenfried · 2021-04-09T10:58:38Z

I discussed this a little bit with @flaktack in the chat window of the last OTP meeting. I think for one-to-many searches we would need to check for loop edges for the entire search, as we do not know if we are close to one of the acceptable destinations.

How would you address this issue?

leonardehrenfried · 2021-04-09T11:06:24Z

@t2gran I've made the testing part of the PR a lot less invasive which makes this a lot easier to review.

@abyrd I've used the method you suggested to compute the "closeness": 0a80bf0

I visualised the envelopes and they produce quite symmetric squares:

gmellemstrand · 2021-04-09T11:27:29Z

I discussed this a little bit with @flaktack in the chat window of the last OTP meeting. I think for one-to-many searches we would need to check for loop edges for the entire search, as we do not know if we are close to one of the acceptable destinations.

How would you address this issue?

I think the isCloseToStartOrEnd should always return true if there are no from/to vertices (and document it on the method). I also see now that you are checking the RoutingRequest.from, when it would probably be more correct to check the RoutingContext.fromVertices and RoutingContext.toVertices. These are now used to search from/to from all stops within a station (which would be in quite close proximity, but there is really no guarantee that they will be).

This would require a check on all from/to vertices, to see whether they are inside the envelopes, but that should not be too expensive.

t2gran

This is almost good to go from a technical point, I have not looked at the functionality.

The addition of the dev-doc is great.

t2gran · 2021-04-06T07:53:26Z

+            if (backEdgeHasTurnRestrictions(a)
+                    /**
+                     * {@link DominanceFunction.MAX_METERS_ROUTE_LOOPS}
+                     */
+                    || a.getBackMode() != TraverseMode.WALK && isCloseToStartOrEnd(a.getVertex(), a.getOptions())) {


JavaDoc inside expression?

I think you commented on an outdated version, right?

t2gran · 2021-04-13T07:45:05Z

+    @DisplayName("car routes can contain loops (traversing the same edge twice)")
+    public void shouldAllowLoopCausedByTurnRestrictions() {
+        var hindenburgStrUnderConstruction = ConstantsForTests.buildOsmGraph(
+                ConstantsForTests.HERRENBERG_HINDENBURG_STR_UNDER_CONSTRUCTION_OSM);


Egyptian: (), break end down.

I used the Intellij code style supplied by you to automatically format this file. Is this not the result you were expecting?

t2gran · 2021-04-13T07:45:51Z

+                computePolyline(hindenburgStrUnderConstruction, gueltsteinerStr, aufDemGraben);
+
+        assertThatPolylinesAreEqual(
+                polyline, "ouqgH}mcu@gAE]U}BaA]Q}@]uAs@[SAm@Ee@AUEi@XEQkBQ?Bz@Dt@Dh@@TGBC@KBSHGx@");


() break end down

t2gran · 2021-04-13T07:46:05Z

+        var itineraries = GraphPathToItineraryMapper.mapItineraries(paths, request);
+        // make sure that we only get CAR legs
+        itineraries.forEach(
+                i -> i.legs.forEach(l -> Assertions.assertEquals(l.mode, TraverseMode.CAR)));


leonardehrenfried · 2021-04-13T10:52:29Z

@t2gran I've reimported the Intellij style but it didn't autoformat to Egyptian brackets so I did it manually.

leonardehrenfried · 2021-04-13T10:53:33Z

@gmellemstrand I've changed the JavaDoc to take the limitations of this approach into account: 23ee8a6

Can you check that I understood everything ok?

t2gran

Thank you for doing this. I reviewed the two last commits, which complete what we talked about in todays developer meeting.

gmellemstrand · 2021-04-14T08:51:18Z

@gmellemstrand I've changed the JavaDoc to take the limitations of this approach into account: 23ee8a6

Can you check that I understood everything ok?

I think what you wrote is sufficient. It gets the point across that the check will have to be expanded if these limitations are not acceptable.

…rt or end

gmellemstrand

Since only documentation and merging adjustments have been done after @t2gran 's review, I will approve this.

leonardehrenfried requested a review from a team April 1, 2021 14:15

leonardehrenfried mentioned this pull request Apr 1, 2021

OTP2 upstreaming status mfdz/OpenTripPlanner#47

Closed

leonardehrenfried changed the title ~~WIP: Car loops~~ WIP: Car loops caused by turn restrictions Apr 1, 2021

leonardehrenfried changed the title ~~WIP: Car loops caused by turn restrictions~~ Car loops caused by turn restrictions Apr 1, 2021

leonardehrenfried commented Apr 1, 2021

View reviewed changes

leonardehrenfried force-pushed the car-loops branch 2 times, most recently from 52bcbb2 to b9ae55f Compare April 1, 2021 15:44

t2gran added this to the 2.1 milestone Apr 1, 2021

leonardehrenfried force-pushed the car-loops branch 2 times, most recently from a8d7c7b to 514d197 Compare April 6, 2021 12:55

leonardehrenfried force-pushed the car-loops branch 8 times, most recently from 9f84369 to 1b0d737 Compare April 9, 2021 08:32

gmellemstrand reviewed Apr 9, 2021

View reviewed changes

leonardehrenfried force-pushed the car-loops branch 2 times, most recently from 415775c to bb6fef7 Compare April 9, 2021 10:58

leonardehrenfried force-pushed the car-loops branch 2 times, most recently from 7d2a8f1 to c3438f9 Compare April 9, 2021 11:08

leonardehrenfried force-pushed the car-loops branch from c3438f9 to 79cee39 Compare April 12, 2021 08:42

t2gran requested changes Apr 13, 2021

View reviewed changes

leonardehrenfried force-pushed the car-loops branch from f93eb3c to 06d779b Compare April 13, 2021 09:48

t2gran previously approved these changes Apr 13, 2021

View reviewed changes

leonardehrenfried added 13 commits April 14, 2021 11:20

Allow loops in routes caused by turn restrictions

1b4f580

Add documentation

7b31a3f

Add changelog

e15e856

Clean up leftovers

af41cda

Improve handling of car-only trips

d669311

Add documentation

f480d89

Add documentation about testing

2e058dd

Use envelopes to performantly compute if a vertex is close to the sta…

ae0dd20

…rt or end

Simplify test graph building

9e1f5c3

Only allow loops when driving

422aafb

Fix graph builder

fa119ec

Reformat brackets

dbd15ad

Document the limitations of the start/end envelope approach

ab68e75

leonardehrenfried dismissed t2gran’s stale review via ab68e75 April 14, 2021 09:23

leonardehrenfried force-pushed the car-loops branch from 23ee8a6 to ab68e75 Compare April 14, 2021 09:23

leonardehrenfried added 2 commits April 14, 2021 11:27

Remove post-merge duplicate method

47e864c

Adjust turn restrictions test to take loops into account

3151219

gmellemstrand approved these changes Apr 14, 2021

View reviewed changes

gmellemstrand merged commit 77d24b6 into opentripplanner:dev-2.x Apr 14, 2021

hbruch deleted the car-loops branch July 28, 2024 08:35

Conversation

leonardehrenfried commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Alternative solutions

Issue

Unit tests

Code style

Documentation

Changelog

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Apr 6, 2021

Test graph building speed

Performance impact

Uh oh!

leonardehrenfried commented Apr 6, 2021

Commands

Results

No loops allowed (current dev-2.x)

Loops are always allowed

Only at start or end

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmellemstrand commented Apr 9, 2021

Uh oh!

leonardehrenfried commented Apr 9, 2021

Uh oh!

leonardehrenfried commented Apr 9, 2021

Uh oh!

gmellemstrand commented Apr 9, 2021

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Apr 13, 2021

Uh oh!

leonardehrenfried commented Apr 13, 2021

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

gmellemstrand commented Apr 14, 2021

Uh oh!

gmellemstrand left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leonardehrenfried commented Apr 1, 2021 •

edited

Loading

No loops allowed (current `dev-2.x`)

leonardehrenfried Apr 9, 2021 •

edited

Loading