Allow multiple states during transfer edge traversals #6238

flaktack · 2024-11-07T08:54:07Z

Summary

(This builds on #6237)

In EdgeTraverser when traversing edge lists multiple states could be returned which was not handled correctly.

This updates `EdgeTraverser˙ and its uses to allow multiple states.

Issue

Unit tests

Unit tests are added for EdgeTraverser. To test the updated transfer and leg traversal a CAR_PICKUP snapshot test is added and then updated.

codecov · 2024-11-07T09:00:30Z

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.72%. Comparing base (cd158d2) to head (4d11307).
Report is 98 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...tripplanner/street/search/state/EdgeTraverser.java	94.73%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6238      +/-   ##
=============================================
+ Coverage      69.71%   69.72%   +0.01%     
- Complexity     17692    17700       +8     
=============================================
  Files           2008     2008              
  Lines          75822    75825       +3     
  Branches        7761     7763       +2     
=============================================
+ Hits           52858    52869      +11     
+ Misses         20250    20245       -5     
+ Partials        2714     2711       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

JS Bundle Analysis - Avoid shipping oversized bundles

leonardehrenfried · 2024-11-14T14:32:47Z

Can you resolve the conflicts?

leonardehrenfried · 2024-11-19T09:20:27Z

...src/main/java/org/opentripplanner/routing/algorithm/mapping/RaptorPathToItineraryMapper.java

-
-      State[] states = transferStates.toArray(new State[0]);
-      var graphPath = new GraphPath<>(states[states.length - 1]);
+      var legTransferSearchRequest = transferStreetRequest


This one didn't use the EdgeTraverser before. What is the reason for using it here?

Multiple states may also occur here, which need to be handled in a similar manner:

there could be multiple initial states

the edge traversal could return multiple states, of which the first one may not be the optimal

As an example, the CAR_PICKUP transfers resulted in an NPE, since the initial CAR/IN_CAR state couldn't traverse the StreetTransitLink entities.

By using EdgeTraverser traversal results in the same state as when creating the actual transfer.

leonardehrenfried · 2024-11-19T09:36:31Z

application/src/main/java/org/opentripplanner/street/search/state/EdgeTraverser.java

-        );
-      }
-      if (State.isEmpty(afterTraversal)) {
+      var vertex = isArriveBy ? e.getToVertex() : e.getFromVertex();


Is the reason we need to care about arriveBy is that we now use the SPT?

Yes, it is due to the SPT which stores states by the vertex. Previously we could ignore the from/to vertex handling since the single previous state was used.

optionsome

Do we support CAR_PICKUP in transfers? Idk how often using one would be realistic.

optionsome · 2024-11-19T15:44:30Z

...ation/src/test/java/org/opentripplanner/routing/algorithm/mapping/CarPickupSnapshotTest.java

@@ -0,0 +1,71 @@
+package org.opentripplanner.routing.algorithm.mapping;
+
+import au.com.origin.snapshots.junit5.SnapshotExtension;


Btw how is this dependency imported into our project?

It's an existing dependency:

OpenTripPlanner/pom.xml

Lines 466 to 470 in 5b5d92f

<dependency>

<groupId>io.github.origin-energy</groupId>

<artifactId>java-snapshot-testing-junit5</artifactId>

<version>2.3.0</version>

</dependency>

optionsome · 2024-11-19T15:46:38Z

...ation/src/test/java/org/opentripplanner/routing/algorithm/mapping/CarPickupSnapshotTest.java

+  }
+
+  @Test
+  public void test_trip_planning_with_car_pickup_only() {


We usually don't use the snake case in tests. I know some people like to use them since they might be slightly more readable. Is there some other reason to use them here?

The existing snapshots tests follow the same format, which I built on:

OpenTripPlanner/application/src/test/java/org/opentripplanner/routing/algorithm/mapping/TransitSnapshotTest.java

Lines 66 to 67 in 5b5d92f

@Test

public void test_trip_planning_with_walk_only() {

We discussed this in the dev meeting today. Apparently we had made some decision a couple of years ago that snake case is not allowed in tests. But since it's used in snapshot tests, we don't think you should change it in the scope of this pr but if you could create a follow up pr where you rename all the snake case snapshot tests, that would be appreciated.

VillePihlava · 2024-11-26T14:52:25Z

Related to the CAR_PICKUP transfer mode, I think it needs to be discussed more, for example, in next Thursday's dev meeting. I'm currently working on splitting the transfers for modes and I ran into this test. Either more changes need to be made to support this (see RequestModesMapper, QualifiedModeSet, and RaptorPathToItineraryMapper), the CAR or WALK mode can be used instead, or some other solution can be figured out. Also, Feature.TRANSFER should be added in the StreetMode class for CAR_PICKUP.

If this was just added for a test, I think the test can be changed to another mode, for example.

flaktack · 2024-11-26T19:40:19Z

CAR_PICKUP was chosen since it returns multiple states. BIKE_RENTAL could be an alternative if the graph contains static rental stations.

VillePihlava · 2024-11-27T10:57:00Z

I tried searching for your name in the OTP gitter chat app, but couldn't find you @flaktack. Can you join the OTP room?

Essentially, I think if this mode is used in a test, then it should also be a real-world use case. To fully implement the mode, the changes I listed in the previous comment should be made. Can CAR_PICKUP or BIKE_RENTAL modes actually be used for transfers? In this case, I can take this into account when working on splitting transfers. I'm under the impression that currently only WALK, BIKE, and CAR (recently added by me, more functionality related to this coming later) modes are valid transfer modes.

I'm not that familiar with multiple states in the transfer edge traversals, but I recall that issue #6210 was caused by the use of the BIKE mode in transfers. Maybe this test could use the BIKE mode as long as the data in the test is appropriate?

leonardehrenfried · 2024-11-27T11:20:48Z

How about we do the following?

@flaktack opens the bike that adds multiple bike states again
@flaktack switches the snapshot test in question to bicycle

That way we don't need to have @VillePihlava straining to fix a test for behaviour that we actually don't want to support.

flaktack · 2024-11-28T07:54:28Z

I should have something ready next week using bike with transit.

flaktack added the Skip Changelog label Nov 7, 2024

t2gran added this to the 2.7 (next release) milestone Nov 12, 2024

t2gran requested review from leonardehrenfried and optionsome November 12, 2024 10:20

flaktack force-pushed the feature/edge-traverser-multiple-states branch from 24c7dc6 to 7ab8e1e Compare November 14, 2024 14:19

flaktack marked this pull request as ready for review November 14, 2024 14:28

flaktack requested a review from a team as a code owner November 14, 2024 14:28

flaktack added 2 commits November 14, 2024 15:31

Add CAR_PICKUP snapshot test

4639734

Street search: traverse edges which may return multiple states

4d11307

flaktack force-pushed the feature/edge-traverser-multiple-states branch from 7ab8e1e to 4d11307 Compare November 14, 2024 14:36

leonardehrenfried reviewed Nov 19, 2024

View reviewed changes

leonardehrenfried approved these changes Nov 19, 2024

View reviewed changes

optionsome reviewed Nov 19, 2024

View reviewed changes

optionsome approved these changes Nov 21, 2024

View reviewed changes

flaktack merged commit a68bc04 into opentripplanner:dev-2.x Nov 22, 2024
5 checks passed

flaktack deleted the feature/edge-traverser-multiple-states branch November 22, 2024 09:17

VillePihlava mentioned this pull request Nov 29, 2024

Enable mode-specific transfers by storing mode information in transfers #6293

Merged

vpaturet mentioned this pull request Jan 3, 2025

Revert allow multiple states during transfer edge traversals #6357

Open

t2gran mentioned this pull request Jan 3, 2025

Generate Raptor transfer cache in parallel #6326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple states during transfer edge traversals #6238

Allow multiple states during transfer edge traversals #6238

flaktack commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

leonardehrenfried commented Nov 14, 2024

leonardehrenfried Nov 19, 2024

flaktack Nov 19, 2024

leonardehrenfried Nov 19, 2024

flaktack Nov 19, 2024

optionsome left a comment

optionsome Nov 19, 2024

flaktack Nov 20, 2024

optionsome Nov 19, 2024 •

edited

Loading

flaktack Nov 20, 2024

optionsome Nov 21, 2024

VillePihlava commented Nov 26, 2024

flaktack commented Nov 26, 2024

VillePihlava commented Nov 27, 2024

leonardehrenfried commented Nov 27, 2024

flaktack commented Nov 28, 2024

		@@ -0,0 +1,71 @@
		package org.opentripplanner.routing.algorithm.mapping;

		import au.com.origin.snapshots.junit5.SnapshotExtension;

	<dependency>
	<groupId>io.github.origin-energy</groupId>
	<artifactId>java-snapshot-testing-junit5</artifactId>
	<version>2.3.0</version>
	</dependency>

Allow multiple states during transfer edge traversals #6238

Allow multiple states during transfer edge traversals #6238

Conversation

flaktack commented Nov 7, 2024 • edited Loading

Summary

Issue

Unit tests

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

leonardehrenfried commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optionsome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optionsome Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava commented Nov 26, 2024

flaktack commented Nov 26, 2024

VillePihlava commented Nov 27, 2024

leonardehrenfried commented Nov 27, 2024

flaktack commented Nov 28, 2024

flaktack commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

optionsome Nov 19, 2024 •

edited

Loading