Skip to content
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.

Sign up for GitHub

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

Allow multiple states during transfer edge traversals #6238

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Allow multiple states during transfer edge traversals #6238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4639734
Add CAR_PICKUP snapshot test
flaktack Nov 6, 2024
4d11307
Street search: traverse edges which may return multiple states
flaktack Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion application/src/ext/java/org/opentripplanner/ext/flex/template/AbstractFlexTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private FlexAccessEgress createFlexAccessEgress(
return null;
}

final var finalStateOpt = EdgeTraverser.traverseEdges(afterFlexState[0], transferEdges);
final var finalStateOpt = EdgeTraverser.traverseEdges(afterFlexState, transferEdges);

return finalStateOpt
.map(finalState -> {
Expand Down
2 changes: 1 addition & 1 deletion application/src/ext/java/org/opentripplanner/ext/flex/template/FlexDirectPathFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ private Optional<DirectFlexPath> createDirectGraphPath(

final State[] afterFlexState = flexEdge.traverse(accessNearbyStop.state);

var finalStateOpt = EdgeTraverser.traverseEdges(afterFlexState[0], egress.edges);
var finalStateOpt = EdgeTraverser.traverseEdges(afterFlexState, egress.edges);

if (finalStateOpt.isEmpty()) {
return Optional.empty();
Expand Down
30 changes: 11 additions & 19 deletions .../main/java/org/opentripplanner/routing/algorithm/mapping/RaptorPathToItineraryMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import org.opentripplanner.astar.model.GraphPath;
import org.opentripplanner.framework.application.OTPFeature;
import org.opentripplanner.framework.geometry.GeometryUtils;
Expand Down Expand Up @@ -38,8 +39,8 @@
import org.opentripplanner.street.search.TraverseMode;
import org.opentripplanner.street.search.request.StreetSearchRequest;
import org.opentripplanner.street.search.request.StreetSearchRequestMapper;
import org.opentripplanner.street.search.state.EdgeTraverser;
import org.opentripplanner.street.search.state.State;
import org.opentripplanner.street.search.state.StateEditor;
import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
import org.opentripplanner.transit.service.TransitService;
Expand Down Expand Up @@ -360,24 +361,15 @@ private List<Leg> mapNonTransitLeg(
.build()
);
} else {
StateEditor se = new StateEditor(edges.get(0).getFromVertex(), transferStreetRequest);
se.setTimeSeconds(createZonedDateTime(pathLeg.fromTime()).toEpochSecond());

State s = se.makeState();
ArrayList<State> transferStates = new ArrayList<>();
transferStates.add(s);
for (Edge e : edges) {
var states = e.traverse(s);
if (State.isEmpty(states)) {
s = null;
} else {
transferStates.add(states[0]);
s = states[0];
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.copyOf(createZonedDateTime(pathLeg.fromTime()).toInstant())
.build();
var initialStates = State.getInitialStates(
Set.of(edges.getFirst().getFromVertex()),
legTransferSearchRequest
);
var state = EdgeTraverser.traverseEdges(initialStates, edges);
var graphPath = new GraphPath<>(state.get());

Itinerary subItinerary = graphPathToItineraryMapper.generateItinerary(graphPath);

Expand Down
9 changes: 4 additions & 5 deletions ...n/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.locationtech.jts.geom.Coordinate;
import org.opentripplanner.raptor.api.model.RaptorCostConverter;
import org.opentripplanner.raptor.api.model.RaptorTransfer;
import org.opentripplanner.routing.api.request.preference.WalkPreferences;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.search.request.StreetSearchRequest;
import org.opentripplanner.street.search.state.EdgeTraverser;
import org.opentripplanner.street.search.state.StateEditor;
import org.opentripplanner.street.search.state.State;
import org.opentripplanner.utils.logging.Throttle;
import org.opentripplanner.utils.tostring.ToStringBuilder;
import org.slf4j.Logger;
Expand Down Expand Up @@ -84,10 +85,8 @@ public Optional<RaptorTransfer> asRaptorTransfer(StreetSearchRequest request) {
);
}

StateEditor se = new StateEditor(edges.get(0).getFromVertex(), request);
se.setTimeSeconds(0);

var state = EdgeTraverser.traverseEdges(se.makeState(), edges);
var initialStates = State.getInitialStates(Set.of(edges.getFirst().getFromVertex()), request);
var state = EdgeTraverser.traverseEdges(initialStates, edges);

return state.map(s ->
new DefaultRaptorTransfer(
Expand Down
4 changes: 4 additions & 0 deletions application/src/main/java/org/opentripplanner/street/search/request/StreetSearchRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ public DataOverlayContext dataOverlayContext() {
return dataOverlayContext;
}

public StreetSearchRequestBuilder copyOf(Instant time) {
return copyOf(this).withStartTime(time);
}

public StreetSearchRequestBuilder copyOfReversed(Instant time) {
return copyOf(this).withStartTime(time).withArriveBy(!arriveBy);
}
Expand Down
58 changes: 43 additions & 15 deletions application/src/main/java/org/opentripplanner/street/search/state/EdgeTraverser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

import java.util.Collection;
import java.util.Optional;
import org.opentripplanner.astar.model.ShortestPathTree;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.model.vertex.Vertex;
import org.opentripplanner.street.search.strategy.DominanceFunctions;

/**
* This is a very reduced version of the A* algorithm: from an initial state a number of edges are
Expand All @@ -14,24 +17,49 @@
*/
public class EdgeTraverser {

public static Optional<State> traverseEdges(final State s, final Collection<Edge> edges) {
var state = s;
public static Optional<State> traverseEdges(
final Collection<State> initialStates,
final Collection<Edge> edges
) {
return traverseEdges(initialStates.toArray(new State[0]), edges);
}

public static Optional<State> traverseEdges(
final State[] initialStates,
final Collection<Edge> edges
) {
if (edges.isEmpty()) {
return Optional.of(initialStates[0]);
}

// The shortest path tree is used to prune dominated parallel states. For example,
// CAR_PICKUP can return both a CAR/WALK state after each traversal of which only
// the optimal states need to be continued.
var dominanceFunction = new DominanceFunctions.MinimumWeight();
var spt = new ShortestPathTree<>(dominanceFunction);
for (State initialState : initialStates) {
spt.add(initialState);
}

Vertex lastVertex = null;
var isArriveBy = initialStates[0].getRequest().arriveBy();
for (Edge e : edges) {
var afterTraversal = e.traverse(state);
if (afterTraversal.length > 1) {
throw new IllegalStateException(
"Expected only a single state returned from edge %s but received %s".formatted(
e,
afterTraversal.length
)
);
}
if (State.isEmpty(afterTraversal)) {
var vertex = isArriveBy ? e.getToVertex() : e.getFromVertex();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var fromStates = spt.getStates(vertex);
if (fromStates == null || fromStates.isEmpty()) {
return Optional.empty();
} else {
state = afterTraversal[0];
}

for (State fromState : fromStates) {
var newToStates = e.traverse(fromState);
for (State newToState : newToStates) {
spt.add(newToState);
}
}

lastVertex = isArriveBy ? e.getFromVertex() : e.getToVertex();
}
return Optional.ofNullable(state);

return Optional.ofNullable(lastVertex).map(spt::getState);
}
}
2 changes: 2 additions & 0 deletions application/src/test/java/org/opentripplanner/_support/geometry/Coordinates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ public class Coordinates {

public static final Coordinate BERLIN = of(52.5212, 13.4105);
public static final Coordinate BERLIN_BRANDENBURG_GATE = of(52.51627, 13.37770);
public static final Coordinate BERLIN_FERNSEHTURM = of(52.52084, 13.40934);
public static final Coordinate BERLIN_ADMIRALBRUCKE = of(52.49526, 13.415093);
public static final Coordinate HAMBURG = of(53.5566, 10.0003);
public static final Coordinate KONGSBERG_PLATFORM_1 = of(59.67216, 9.65107);
public static final Coordinate BOSTON = of(42.36541, -71.06129);
Expand Down
71 changes: 71 additions & 0 deletions ...on/src/test/java/org/opentripplanner/routing/algorithm/mapping/CarPickupSnapshotTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package org.opentripplanner.routing.algorithm.mapping;

import au.com.origin.snapshots.junit5.SnapshotExtension;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw how is this dependency imported into our project?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

import java.util.List;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.opentripplanner.model.GenericLocation;
import org.opentripplanner.model.modes.ExcludeAllTransitFilter;
import org.opentripplanner.routing.api.request.RouteRequest;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.routing.api.request.request.filter.AllowAllTransitFilter;

@ExtendWith(SnapshotExtension.class)
public class CarPickupSnapshotTest extends SnapshotTestBase {

static GenericLocation p0 = new GenericLocation(
"SE Stark St. & SE 17th Ave. (P0)",
null,
45.519320,
-122.648567
);

static GenericLocation p1 = new GenericLocation(
"SE Morrison St. & SE 17th Ave. (P1)",
null,
45.51726,
-122.64847
);

static GenericLocation p2 = new GenericLocation(
"NW Northrup St. & NW 22nd Ave. (P2)",
null,
45.53122,
-122.69659
);

@BeforeAll
public static void beforeClass() {
loadGraphBeforeClass(false);
}

@Test
public void test_trip_planning_with_car_pickup_only() {
Copy link
Member

@optionsome optionsome Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RouteRequest request = createTestRequest(2009, 11, 17, 10, 0, 0);

request.journey().direct().setMode(StreetMode.CAR_PICKUP);
request.journey().transit().setFilters(List.of(ExcludeAllTransitFilter.of()));

request.setFrom(p0);
request.setTo(p2);

expectRequestResponseToMatchSnapshot(request);
}

@Test
public void test_trip_planning_with_car_pickup_transfer() {
RouteRequest request = createTestRequest(2009, 11, 17, 10, 0, 0);

request.journey().access().setMode(StreetMode.WALK);
request.journey().egress().setMode(StreetMode.WALK);
request.journey().direct().setMode(StreetMode.WALK);
request.journey().transfer().setMode(StreetMode.CAR_PICKUP);
request.journey().transit().setFilters(List.of(AllowAllTransitFilter.of()));

request.setFrom(p0);
request.setTo(p2);

expectRequestResponseToMatchSnapshot(request);
}
}
Loading
Loading