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

Add 'transferParametersForMode' build config field #6215

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
36336bf
Add ability to create special transfer requests for cars and bikes wi…
VillePihlava Nov 1, 2024
f1380e3
Add tests for 'carsAllowedStopMaxTransferDurationsForMode' build conf…
VillePihlava Nov 1, 2024
2175e64
Move if statement outside of for loop.
VillePihlava Nov 1, 2024
6418e26
Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …
VillePihlava Nov 5, 2024
e1e9bbb
Fixes based on review comments.
VillePihlava Dec 3, 2024
89e617f
Add TransferParameters to build config.
VillePihlava Dec 3, 2024
a1e0807
Remove mentions of carsAllowedStopMaxTransferDurationsForMode and fix…
VillePihlava Dec 3, 2024
21bb307
Rename variables and small improvements.
VillePihlava Dec 5, 2024
573078a
Change test.
VillePihlava Dec 5, 2024
3fd5bf5
Add documentation for build config fields.
VillePihlava Dec 5, 2024
35c98bc
Add logging for transfers.
VillePihlava Dec 5, 2024
3a34aa4
Merge branch 'split-transfers-by-mode-pathtransfer-mode' into car-tra…
VillePihlava Dec 5, 2024
97238a3
Fix merge issues.
VillePihlava Dec 5, 2024
41a0674
Add tests and small improvements.
VillePihlava Dec 10, 2024
1a3f846
Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …
VillePihlava Dec 16, 2024
2e048f9
Refactor transfer parameter parsing.
VillePihlava Dec 16, 2024
c90c3ec
Remove duplicate test.
VillePihlava Dec 16, 2024
30cc925
Simplify implementation by removing changes to StreetNearbyStopFinder.
VillePihlava Dec 17, 2024
2e57c2f
Remove unnecessary parameter from function call.
VillePihlava Dec 17, 2024
f2296cd
Add formatting for JSON in documentation.
VillePihlava Jan 7, 2025
0ac172e
Rename variables.
VillePihlava Jan 7, 2025
bff77a5
Refactor transfer generation.
VillePihlava Jan 7, 2025
9f5d63c
Rename transfers to transferParameters in the build config and throw …
VillePihlava Jan 7, 2025
4f84804
Change wording of documentation.
VillePihlava Jan 7, 2025
4d599a2
Add comments and refactor transfer generation.
VillePihlava Jan 7, 2025
2eb0912
Rename transfers to transferParameters in documentation.
VillePihlava Jan 7, 2025
57e03b4
Change default values from Duration.ZERO to null.
VillePihlava Jan 14, 2025
7de4a39
Add documentation.
VillePihlava Jan 14, 2025
8e87974
Rename transferParameters to transferParametersForMode.
VillePihlava Jan 14, 2025
d0fbae9
Change documentation for maxTransferDuration.
VillePihlava Jan 14, 2025
f385061
Add documentation to standalone build-config.json and NodeAdapterHelper.
VillePihlava Jan 14, 2025
afa1b16
Use method to get field instead of the field directly.
VillePihlava Jan 16, 2025
c649ab7
Change comment style.
VillePihlava Jan 16, 2025
91782ef
Split transfer generation into methods.
VillePihlava Jan 16, 2025
186d20c
Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …
VillePihlava Jan 21, 2025
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 @@ -126,10 +126,8 @@ 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);
mBuilder.withDirectMode(StreetMode.CAR);
// This is used in transfer cache request calculations.
mBuilder.withAllStreetModes(StreetMode.CAR);
Copy link
Member

Choose a reason for hiding this comment

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

I know this builder method wasn't introduced in this pr but this name is really confusing. Idk what we call Access+Transfer+Egress+Direct. It's difficult to come up with a name that doesn't confuse you into thinking that all modes are enabled. Just making this singular would maybe make it less confusing.

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 set each mode separately for more clarity

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@
import com.google.common.collect.HashMultimap;
import com.google.common.collect.Multimaps;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
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,10 +21,13 @@
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.api.request.framework.DurationForEnum;
import org.opentripplanner.routing.graph.Graph;
import org.opentripplanner.routing.graphfinder.NearbyStop;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.model.vertex.TransitStopVertex;
import org.opentripplanner.street.model.vertex.Vertex;
import org.opentripplanner.transit.model.site.RegularStop;
import org.opentripplanner.transit.model.site.StopLocation;
import org.opentripplanner.transit.service.DefaultTransitService;
Expand All @@ -44,6 +51,7 @@ public class DirectTransferGenerator implements GraphBuilderModule {
private final Duration radiusByDuration;

private final List<RouteRequest> transferRequests;
private final DurationForEnum<StreetMode> carsAllowedStopMaxTransferDurationsForMode;
private final Graph graph;
private final TimetableRepository timetableRepository;
private final DataImportIssueStore issueStore;
Expand All @@ -60,6 +68,23 @@ public DirectTransferGenerator(
this.issueStore = issueStore;
this.radiusByDuration = radiusByDuration;
this.transferRequests = transferRequests;
this.carsAllowedStopMaxTransferDurationsForMode = DurationForEnum.of(StreetMode.class).build();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should always include this with some default value that is the same as the normal transfer duration limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has changed quite a lot, but you can look at how I handle an empty parameter

}

public DirectTransferGenerator(
Graph graph,
TimetableRepository timetableRepository,
DataImportIssueStore issueStore,
Duration radiusByDuration,
List<RouteRequest> transferRequests,
DurationForEnum<StreetMode> carsAllowedStopMaxTransferDurationsForMode
) {
this.graph = graph;
this.timetableRepository = timetableRepository;
this.issueStore = issueStore;
this.radiusByDuration = radiusByDuration;
this.transferRequests = transferRequests;
this.carsAllowedStopMaxTransferDurationsForMode = carsAllowedStopMaxTransferDurationsForMode;
}

@Override
Expand All @@ -68,9 +93,16 @@ public void buildGraph() {
timetableRepository.index();

/* The linker will use streets if they are available, or straight-line distance otherwise. */
NearbyStopFinder nearbyStopFinder = createNearbyStopFinder();
NearbyStopFinder nearbyStopFinder = createNearbyStopFinder(radiusByDuration, Set.of());

List<TransitStopVertex> stops = graph.getVerticesOfType(TransitStopVertex.class);
Set<TransitStopVertex> carsAllowedStops = timetableRepository
.getStopLocationsUsedForCarsAllowedTrips()
.stream()
.map(StopLocation::getId)
.map(graph::getStopVertexForStopId)
.filter(TransitStopVertex.class::isInstance) // filter out null values if no TransitStopVertex is found for ID
Copy link
Member

Choose a reason for hiding this comment

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

Inline comments are not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

.collect(Collectors.toSet());

ProgressTracker progress = ProgressTracker.track(
"Create transfer edges for stops",
Expand All @@ -86,6 +118,37 @@ public void buildGraph() {
HashMultimap.create()
);

List<RouteRequest> filteredTransferRequests = new ArrayList<RouteRequest>();
List<RouteRequest> carsAllowedStopTransferRequests = new ArrayList<RouteRequest>();
HashMap<StreetMode, NearbyStopFinder> carsAllowedStopNearbyStopFinders = new HashMap<>();

// Split transfer requests into normal and carsAllowedStop requests.
for (RouteRequest transferProfile : transferRequests) {
StreetMode mode = transferProfile.journey().transfer().mode();
if (carsAllowedStopMaxTransferDurationsForMode.containsKey(mode)) {
carsAllowedStopNearbyStopFinders.put(
mode,
createNearbyStopFinder(
carsAllowedStopMaxTransferDurationsForMode.valueOf(mode),
Collections.<Vertex>unmodifiableSet(carsAllowedStops)
)
);

carsAllowedStopTransferRequests.add(transferProfile);
// For bikes, also normal transfer requests are wanted.
if (mode == StreetMode.BIKE) {
filteredTransferRequests.add(transferProfile);
}
} else if (mode == StreetMode.CAR) {
// Special transfers are always created for cars.
// If a duration is not specified for cars, the default is used.
carsAllowedStopNearbyStopFinders.put(mode, nearbyStopFinder);
carsAllowedStopTransferRequests.add(transferProfile);
} else {
filteredTransferRequests.add(transferProfile);
}
}

stops
.stream()
.parallel()
Expand All @@ -101,26 +164,11 @@ public void buildGraph() {

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

for (RouteRequest transferProfile : transferRequests) {
for (NearbyStop sd : nearbyStopFinder.findNearbyStops(
ts0,
transferProfile,
transferProfile.journey().transfer(),
false
)) {
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
continue;
}
if (sd.stop.transfersNotAllowed()) {
continue;
}
distinctTransfers.put(
new TransferKey(stop, sd.stop, sd.edges),
new PathTransfer(stop, sd.stop, sd.distance, sd.edges)
);
}
if (OTPFeature.FlexRouting.isOn()) {
for (RouteRequest transferProfile : filteredTransferRequests) {
findNearbyStops(nearbyStopFinder, ts0, transferProfile, stop, distinctTransfers);
}
if (OTPFeature.FlexRouting.isOn()) {
for (RouteRequest transferProfile : filteredTransferRequests) {
// 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(
Expand All @@ -144,6 +192,22 @@ public void buildGraph() {
}
}

// This calculates transfers between stops that can use trips with cars.
for (RouteRequest transferProfile : carsAllowedStopTransferRequests) {
StreetMode mode = transferProfile.journey().transfer().mode();
if (
carsAllowedStops.contains(ts0) && carsAllowedStopNearbyStopFinders.containsKey(mode)
) {
findNearbyStops(
carsAllowedStopNearbyStopFinders.get(mode),
ts0,
transferProfile,
stop,
distinctTransfers
);
}
}

LOG.debug(
"Linked stop {} with {} transfers to stops with different patterns.",
stop,
Expand Down Expand Up @@ -179,7 +243,10 @@ public void buildGraph() {
* whether the graph has a street network and if ConsiderPatternsForDirectTransfers feature is
* enabled.
*/
private NearbyStopFinder createNearbyStopFinder() {
private NearbyStopFinder createNearbyStopFinder(
Duration radiusByDuration,
Set<Vertex> findOnlyVertices
) {
var transitService = new DefaultTransitService(timetableRepository);
NearbyStopFinder finder;
if (!graph.hasStreets) {
Expand All @@ -189,7 +256,7 @@ private NearbyStopFinder createNearbyStopFinder() {
finder = new StraightLineNearbyStopFinder(transitService, radiusByDuration);
} else {
LOG.info("Creating direct transfer edges between stops using the street network from OSM...");
finder = new StreetNearbyStopFinder(radiusByDuration, 0, null);
finder = new StreetNearbyStopFinder(radiusByDuration, 0, null, Set.of(), findOnlyVertices);
}

if (OTPFeature.ConsiderPatternsForDirectTransfers.isOn()) {
Expand All @@ -199,5 +266,32 @@ private NearbyStopFinder createNearbyStopFinder() {
}
}

private void findNearbyStops(
NearbyStopFinder nearbyStopFinder,
TransitStopVertex ts0,
RouteRequest transferProfile,
RegularStop stop,
Map<TransferKey, PathTransfer> distinctTransfers
) {
for (NearbyStop sd : nearbyStopFinder.findNearbyStops(
ts0,
transferProfile,
transferProfile.journey().transfer(),
false
)) {
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
continue;
}
if (sd.stop.transfersNotAllowed()) {
continue;
}
distinctTransfers.put(
new TransferKey(stop, sd.stop, sd.edges),
new PathTransfer(stop, sd.stop, sd.distance, sd.edges)
);
}
}

private record TransferKey(StopLocation source, StopLocation target, List<Edge> edges) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ static DirectTransferGenerator provideDirectTransferGenerator(
timetableRepository,
issueStore,
config.maxTransferDuration,
config.transferRequests
config.transferRequests,
config.carsAllowedStopMaxTransferDurationsForMode
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
import org.opentripplanner.street.model.vertex.Vertex;
import org.opentripplanner.street.search.StreetSearchBuilder;
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.State;
import org.opentripplanner.street.search.strategy.DominanceFunctions;
import org.opentripplanner.transit.model.site.AreaStop;
Expand All @@ -42,6 +40,7 @@ public class StreetNearbyStopFinder implements NearbyStopFinder {
private final int maxStopCount;
private final DataOverlayContext dataOverlayContext;
private final Set<Vertex> ignoreVertices;
private final Set<Vertex> findOnlyVertices;

/**
* Construct a NearbyStopFinder for the given graph and search radius.
Expand All @@ -54,7 +53,7 @@ public StreetNearbyStopFinder(
int maxStopCount,
DataOverlayContext dataOverlayContext
) {
this(durationLimit, maxStopCount, dataOverlayContext, Set.of());
this(durationLimit, maxStopCount, dataOverlayContext, Set.of(), Set.of());
}

/**
Expand All @@ -69,11 +68,31 @@ public StreetNearbyStopFinder(
int maxStopCount,
DataOverlayContext dataOverlayContext,
Set<Vertex> ignoreVertices
) {
this(durationLimit, maxStopCount, dataOverlayContext, ignoreVertices, Set.of());
}

/**
* Construct a NearbyStopFinder for the given graph and search radius.
*
* @param maxStopCount The maximum stops to return. 0 means no limit. Regardless of the maxStopCount
* we will always return all the directly connected stops.
* @param ignoreVertices A set of stop vertices to ignore and not return NearbyStops for.
*
* @param findOnlyVertices Only return NearbyStops that are in this set. If this is empty, no filtering is performed.
*/
public StreetNearbyStopFinder(
Duration durationLimit,
int maxStopCount,
DataOverlayContext dataOverlayContext,
Set<Vertex> ignoreVertices,
Set<Vertex> findOnlyVertices
) {
this.dataOverlayContext = dataOverlayContext;
this.durationLimit = durationLimit;
this.maxStopCount = maxStopCount;
this.ignoreVertices = ignoreVertices;
this.findOnlyVertices = findOnlyVertices;
}

/**
Expand Down Expand Up @@ -146,7 +165,10 @@ public Collection<NearbyStop> findNearbyStops(
continue;
}
if (targetVertex instanceof TransitStopVertex tsv && state.isFinal()) {
stopsFound.add(NearbyStop.nearbyStopForState(state, tsv.getStop()));
// If a set of findOnlyVertices is provided, only add stops that are in the set.
if (findOnlyVertices.isEmpty() || findOnlyVertices.contains(targetVertex)) {
stopsFound.add(NearbyStop.nearbyStopForState(state, tsv.getStop()));
}
}
if (
OTPFeature.FlexRouting.isOn() &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public Duration defaultValue() {
return defaultValue;
}

public boolean containsKey(E key) {
return valueForEnum.containsKey(key);
}

/**
* Utility method to get {@link #defaultValue} as an number in unit seconds. Equivalent to
* {@code (int) defaultValue.toSeconds()}. The downcast is safe since we only allow days, hours,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@
import org.opentripplanner.model.calendar.ServiceDateInterval;
import org.opentripplanner.netex.config.NetexFeedParameters;
import org.opentripplanner.routing.api.request.RouteRequest;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.routing.api.request.framework.DurationForEnum;
import org.opentripplanner.routing.fares.FareServiceFactory;
import org.opentripplanner.standalone.config.buildconfig.CarsAllowedStopMaxTransferDurationsForMode;
import org.opentripplanner.standalone.config.buildconfig.DemConfig;
import org.opentripplanner.standalone.config.buildconfig.GtfsConfig;
import org.opentripplanner.standalone.config.buildconfig.IslandPruningConfig;
Expand Down Expand Up @@ -151,6 +154,7 @@ public class BuildConfig implements OtpDataStoreConfig {
public final IslandPruningConfig islandPruning;

public final Duration maxTransferDuration;
public final DurationForEnum<StreetMode> carsAllowedStopMaxTransferDurationsForMode;
public final NetexFeedParameters netexDefaults;
public final GtfsFeedParameters gtfsDefaults;

Expand Down Expand Up @@ -287,6 +291,12 @@ When set to true (it is false by default), the elevation module will include the
"Transfers up to this duration with the default walk speed value will be pre-calculated and included in the Graph."
)
.asDuration(Duration.ofMinutes(30));
carsAllowedStopMaxTransferDurationsForMode =
CarsAllowedStopMaxTransferDurationsForMode.map(
root,
"carsAllowedStopMaxTransferDurationsForMode",
maxTransferDuration
);
maxStopToShapeSnapDistance =
root
.of("maxStopToShapeSnapDistance")
Expand Down
Loading
Loading