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.

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

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

Open
wants to merge 13 commits into
base: dev-2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
import com.google.common.collect.Multimap;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.opentripplanner.ext.flex.trip.FlexTrip;
import org.opentripplanner.model.PathTransfer;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.transit.model.framework.FeedScopedId;
import org.opentripplanner.transit.model.network.Route;
import org.opentripplanner.transit.model.site.GroupStop;
Expand All @@ -18,15 +20,21 @@ public class FlexIndex {

private final Multimap<StopLocation, PathTransfer> transfersToStop = ArrayListMultimap.create();

private final Multimap<StopLocation, PathTransfer> transfersFromStop = ArrayListMultimap.create();

private final Multimap<StopLocation, FlexTrip<?, ?>> flexTripsByStop = HashMultimap.create();

private final Map<FeedScopedId, Route> routeById = new HashMap<>();

private final Map<FeedScopedId, FlexTrip<?, ?>> tripById = new HashMap<>();

public FlexIndex(TimetableRepository timetableRepository) {
for (PathTransfer transfer : timetableRepository.getAllPathTransfers()) {
// Flex transfers should only use WALK mode transfers.
StreetMode mode = StreetMode.WALK;
List<PathTransfer> filteredPathTransfers = timetableRepository.getAllPathTransfers().stream().filter(pathTransfer -> pathTransfer.getModes().contains(mode)).toList();
for (PathTransfer transfer : filteredPathTransfers) {
transfersToStop.put(transfer.to, transfer);
transfersFromStop.put(transfer.from, transfer);
leonardehrenfried marked this conversation as resolved.
Show resolved Hide resolved
}
for (FlexTrip<?, ?> flexTrip : timetableRepository.getAllFlexTrips()) {
routeById.put(flexTrip.getTrip().getRoute().getId(), flexTrip.getTrip().getRoute());
Expand All @@ -47,6 +55,10 @@ public Collection<PathTransfer> getTransfersToStop(StopLocation stopLocation) {
return transfersToStop.get(stopLocation);
}

public Collection<PathTransfer> getTransfersFromStop(StopLocation stopLocation) {
return transfersFromStop.get(stopLocation);
}

public Collection<FlexTrip<?, ?>> getFlexTripsByStop(StopLocation stopLocation) {
return flexTripsByStop.get(stopLocation);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ public TransitStopVertex getStopVertexForStopId(FeedScopedId stopId) {

@Override
public Collection<PathTransfer> getTransfersFromStop(StopLocation stop) {
return transitService.findPathTransfers(stop);
return transitService.getFlexIndex().getTransfersFromStop(stop);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,10 @@ public RequestModes getRequestModes() {
mBuilder.withEgressMode(StreetMode.CAR_HAILING);
mBuilder.withDirectMode(StreetMode.WALK);
} else {
mBuilder.withAccessMode(StreetMode.WALK);
mBuilder.withTransferMode(StreetMode.WALK);
mBuilder.withEgressMode(StreetMode.WALK);
// This is necessary for transfer calculations.
mBuilder.withAccessMode(StreetMode.CAR);
mBuilder.withTransferMode(StreetMode.CAR);
mBuilder.withEgressMode(StreetMode.CAR);
mBuilder.withDirectMode(StreetMode.CAR);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
import com.google.common.collect.HashMultimap;
import com.google.common.collect.Multimaps;
import java.time.Duration;
import java.util.ArrayList;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
import org.opentripplanner.framework.application.OTPFeature;
import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
import org.opentripplanner.graph_builder.issues.StopNotLinkedForTransfers;
Expand All @@ -17,6 +20,7 @@
import org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinder;
import org.opentripplanner.model.PathTransfer;
import org.opentripplanner.routing.api.request.RouteRequest;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.routing.graph.Graph;
import org.opentripplanner.routing.graphfinder.NearbyStop;
import org.opentripplanner.street.model.edge.Edge;
Expand Down Expand Up @@ -86,6 +90,17 @@ public void buildGraph() {
HashMultimap.create()
);

List<RouteRequest> flexTransferRequests = new ArrayList<>();
// Flex transfer requests only use the WALK mode.
if (OTPFeature.FlexRouting.isOn()) {
flexTransferRequests.addAll(
transferRequests
.stream()
.filter(transferProfile -> transferProfile.journey().transfer().mode() == StreetMode.WALK)
.toList()
);
}

stops
.stream()
.parallel()
Expand All @@ -101,7 +116,9 @@ public void buildGraph() {

LOG.debug("Linking stop '{}' {}", stop, ts0);

// Calculate default transfers.
for (RouteRequest transferProfile : transferRequests) {
StreetMode mode = transferProfile.journey().transfer().mode();
for (NearbyStop sd : nearbyStopFinder.findNearbyStops(
ts0,
transferProfile,
Expand All @@ -115,31 +132,51 @@ public void buildGraph() {
if (sd.stop.transfersNotAllowed()) {
continue;
}
distinctTransfers.put(
new TransferKey(stop, sd.stop, sd.edges),
new PathTransfer(stop, sd.stop, sd.distance, sd.edges)
);
TransferKey transferKey = new TransferKey(stop, sd.stop, sd.edges);
PathTransfer pathTransfer = distinctTransfers.get(transferKey);
if (pathTransfer == null) {
// If the PathTransfer can't be found, it is created.
distinctTransfers.put(
transferKey,
new PathTransfer(stop, sd.stop, sd.distance, sd.edges, EnumSet.of(mode))
);
} else {
// If the PathTransfer is found, a new PathTransfer with the added mode is created.
distinctTransfers.put(transferKey, pathTransfer.withAddedMode(mode));
}
}
if (OTPFeature.FlexRouting.isOn()) {
// This code is for finding transfers from AreaStops to Stops, transfers
// from Stops to AreaStops and between Stops are already covered above.
for (NearbyStop sd : nearbyStopFinder.findNearbyStops(
ts0,
transferProfile,
transferProfile.journey().transfer(),
true
)) {
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
continue;
}
if (sd.stop instanceof RegularStop) {
continue;
}
}
// Calculate flex transfers if flex routing is enabled.
for (RouteRequest transferProfile : flexTransferRequests) {
// Flex transfer requests only use the WALK mode.
StreetMode mode = StreetMode.WALK;
// This code is for finding transfers from AreaStops to Stops, transfers
// from Stops to AreaStops and between Stops are already covered above.
for (NearbyStop sd : nearbyStopFinder.findNearbyStops(
ts0,
transferProfile,
transferProfile.journey().transfer(),
true
)) {
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
continue;
}
if (sd.stop instanceof RegularStop) {
continue;
}
// The TransferKey and PathTransfer are created differently for flex routing.
TransferKey transferKey = new TransferKey(sd.stop, stop, sd.edges);
PathTransfer pathTransfer = distinctTransfers.get(transferKey);
if (pathTransfer == null) {
// If the PathTransfer can't be found, it is created.
distinctTransfers.put(
new TransferKey(sd.stop, stop, sd.edges),
new PathTransfer(sd.stop, stop, sd.distance, sd.edges)
transferKey,
new PathTransfer(sd.stop, stop, sd.distance, sd.edges, EnumSet.of(mode))
);
} else {
// If the PathTransfer is found, a new PathTransfer with the added mode is created.
distinctTransfers.put(transferKey, pathTransfer.withAddedMode(mode));
}
}
}
Expand Down Expand Up @@ -172,6 +209,21 @@ public void buildGraph() {
nTransfersTotal,
nLinkedStops
);
for (StreetMode mode : transferRequests
.stream()
.map(transferProfile -> transferProfile.journey().transfer().mode())
.distinct()
.toList()) {
LOG.info(
"Created {} transfers for mode {}.",
transfersByStop
.values()
.stream()
.filter(pathTransfer -> pathTransfer.getModes().contains(mode))
.count(),
mode
);
Copy link
Member

@optionsome optionsome Dec 10, 2024

Choose a reason for hiding this comment

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

This is quite difficult to read. I would probably continue the stream instead of making it a list and then looping over it, and then call some method with the mode as an argument that does the logging. So something like:

transferRequests
      .stream()
      .map(transferProfile -> transferProfile.journey().transfer().mode())
      .distinct()
      .peek(mode -> logTransfersForMode(mode));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to this:

transferRequests
      .stream()
      .map(transferProfile -> transferProfile.journey().transfer().mode())
      .distinct()
      .forEach(mode ->
        LOG.info(
          "Created {} transfers for mode {}.",
          timetableRepository.getTransfersByMode(mode).size(),
          mode
        )
      );

}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
package org.opentripplanner.model;

import java.io.Serializable;
import java.util.EnumSet;
import java.util.List;
import org.opentripplanner.model.transfer.ConstrainedTransfer;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.transit.model.site.StopLocation;
import org.opentripplanner.utils.tostring.ToStringBuilder;

/**
* Represents a transfer between stops with the street network path attatched to it.
* Represents a transfer for a set of modes between stops with the street network path attached to it.
* <p>
* Do not confuse this with {@link ConstrainedTransfer}.
*
* <p>
* TODO these should really have a set of valid modes in case bike vs. walk transfers are different
* TODO Should we just store the NearbyStop as a field here, or even switch to using it instead
* where this class is used
*/
Expand All @@ -27,11 +28,20 @@ public class PathTransfer implements Serializable {

private final List<Edge> edges;

public PathTransfer(StopLocation from, StopLocation to, double distanceMeters, List<Edge> edges) {
private final EnumSet<StreetMode> modes;

public PathTransfer(
StopLocation from,
StopLocation to,
double distanceMeters,
List<Edge> edges,
EnumSet<StreetMode> modes
) {
this.from = from;
this.to = to;
this.distanceMeters = distanceMeters;
this.edges = edges;
this.modes = modes;
}

public String getName() {
Expand All @@ -43,7 +53,17 @@ public double getDistanceMeters() {
}

public List<Edge> getEdges() {
return this.edges;
return edges;
}

public EnumSet<StreetMode> getModes() {
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to expose the mutable EnumSet. Can you replace it with a method like allows(CAR)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the getModes method to clone the enum set

return modes.clone();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return modes.clone();
return EnumSet.copyOf(modes);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this

}

public PathTransfer withAddedMode(StreetMode mode) {
EnumSet<StreetMode> newModes = EnumSet.copyOf(modes);
newModes.add(mode);
return new PathTransfer(from, to, distanceMeters, edges, newModes);
}

@Override
Expand All @@ -54,6 +74,7 @@ public String toString() {
.addObj("to", to)
.addNum("distance", distanceMeters)
.addColSize("edges", edges)
.addColSize("modes", modes)
.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ else if (pathLeg.isTransferLeg()) {
legs.addAll(
mapTransferLeg(
pathLeg.asTransferLeg(),
transferMode == StreetMode.BIKE ? TraverseMode.BICYCLE : TraverseMode.WALK
StreetModeToTransferTraverseModeMapper.map(transferMode)
)
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.opentripplanner.routing.algorithm.mapping;

import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.street.search.TraverseMode;

/**
* Maps street mode to transfer traverse mode.
*/
public class StreetModeToTransferTraverseModeMapper {
Copy link
Member

Choose a reason for hiding this comment

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

Nice that you extraced the mapper.


public static TraverseMode map(StreetMode mode) {
return switch (mode) {
case WALK -> TraverseMode.WALK;
case BIKE -> TraverseMode.BICYCLE;
case CAR -> TraverseMode.CAR;
default -> throw new IllegalArgumentException(
String.format("StreetMode %s can not be mapped to a TraverseMode for transfers.", mode)
);
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.List;
import java.util.function.Function;
import org.opentripplanner.raptor.api.model.RaptorTransfer;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.street.search.request.StreetSearchRequest;

public class RaptorTransferIndex {
Expand All @@ -29,6 +30,7 @@ public static RaptorTransferIndex create(
) {
var forwardTransfers = new ArrayList<List<RaptorTransfer>>(transfersByStopIndex.size());
var reversedTransfers = new ArrayList<List<RaptorTransfer>>(transfersByStopIndex.size());
StreetMode mode = request.mode();

for (int i = 0; i < transfersByStopIndex.size(); i++) {
forwardTransfers.add(new ArrayList<>());
Expand All @@ -41,6 +43,7 @@ public static RaptorTransferIndex create(
var transfers = transfersByStopIndex
.get(fromStop)
.stream()
.filter(transfer -> transfer.getModes().contains(mode))
Copy link
Member

Choose a reason for hiding this comment

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

This is run only once and then cached for the specific combination of request parameters, correct?

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, this is run once for each cache request. You can see the code that uses this in RaptorRequestTransferCache

.flatMap(s -> s.asRaptorTransfer(request).stream())
.collect(
toMap(RaptorTransfer::stop, Function.identity(), (a, b) -> a.c1() < b.c1() ? a : b)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.EnumSet;
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.StreetMode;
import org.opentripplanner.routing.api.request.preference.WalkPreferences;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.search.request.StreetSearchRequest;
Expand All @@ -31,16 +33,20 @@ public class Transfer {

private final List<Edge> edges;

public Transfer(int toStop, List<Edge> edges) {
private final EnumSet<StreetMode> modes;

public Transfer(int toStop, List<Edge> edges, EnumSet<StreetMode> modes) {
this.toStop = toStop;
this.edges = edges;
this.distanceMeters = (int) edges.stream().mapToDouble(Edge::getDistanceMeters).sum();
this.modes = modes;
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to make this immutable. EnumSet are not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the getModes method to clone the enum set

}

public Transfer(int toStopIndex, int distanceMeters) {
public Transfer(int toStopIndex, int distanceMeters, EnumSet<StreetMode> modes) {
this.toStop = toStopIndex;
this.distanceMeters = distanceMeters;
this.edges = null;
this.modes = modes;
Copy link
Member

Choose a reason for hiding this comment

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

This transfer is not serialized. Here you can use Sets.immutableEnumSet and then never have to worry about cloning or copying.

Copy link
Contributor Author

@VillePihlava VillePihlava Dec 9, 2024

Choose a reason for hiding this comment

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

I changed this to use Sets.immutableEnumSet

}

public List<Coordinate> getCoordinates() {
Expand Down Expand Up @@ -68,6 +74,10 @@ public List<Edge> getEdges() {
return edges;
}

public EnumSet<StreetMode> getModes() {
return modes.clone();
}

public Optional<RaptorTransfer> asRaptorTransfer(StreetSearchRequest request) {
WalkPreferences walkPreferences = request.preferences().walk();
if (edges == null || edges.isEmpty()) {
Expand Down
Loading
Loading