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 'transfers' build config field #6215

Open
wants to merge 19 commits into
base: dev-2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 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
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 @@ -4,10 +4,12 @@
import com.google.common.collect.Multimaps;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collections;
import java.util.EnumSet;
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;
Expand All @@ -25,6 +27,7 @@
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 @@ -45,9 +48,10 @@ public class DirectTransferGenerator implements GraphBuilderModule {

private static final Logger LOG = LoggerFactory.getLogger(DirectTransferGenerator.class);

private final Duration radiusByDuration;
private final Duration defaultMaxTransferDuration;

private final List<RouteRequest> transferRequests;
private final Map<StreetMode, TransferParameters> transferParametersForMode;
private final Graph graph;
private final TimetableRepository timetableRepository;
private final DataImportIssueStore issueStore;
Expand All @@ -56,14 +60,31 @@ public DirectTransferGenerator(
Graph graph,
Copy link
Member

Choose a reason for hiding this comment

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

Is this constructor used only for testing? If so, can you add Javadoc?

TimetableRepository timetableRepository,
DataImportIssueStore issueStore,
Duration radiusByDuration,
Duration defaultMaxTransferDuration,
List<RouteRequest> transferRequests
) {
this.graph = graph;
this.timetableRepository = timetableRepository;
this.issueStore = issueStore;
this.radiusByDuration = radiusByDuration;
this.defaultMaxTransferDuration = defaultMaxTransferDuration;
this.transferRequests = transferRequests;
this.transferParametersForMode = Collections.emptyMap();
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
this.transferParametersForMode = Collections.emptyMap();
this.transferParametersForMode = Map.of();

}

public DirectTransferGenerator(
Graph graph,
TimetableRepository timetableRepository,
DataImportIssueStore issueStore,
Duration defaultMaxTransferDuration,
List<RouteRequest> transferRequests,
Map<StreetMode, TransferParameters> transferParametersForMode
) {
this.graph = graph;
this.timetableRepository = timetableRepository;
this.issueStore = issueStore;
this.defaultMaxTransferDuration = defaultMaxTransferDuration;
this.transferRequests = transferRequests;
this.transferParametersForMode = transferParametersForMode;
}

@Override
Expand All @@ -72,9 +93,24 @@ 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(defaultMaxTransferDuration);
HashMap<StreetMode, NearbyStopFinder> defaultNearbyStopFinders = new HashMap<>();
/* These are used for calculating transfers only between carsAllowedStops. */
HashMap<StreetMode, NearbyStopFinder> carsAllowedStopNearbyStopFinders = new HashMap<>();
Comment on lines +97 to +99
Copy link
Member

Choose a reason for hiding this comment

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

I would call these xNearbyStopFinderForMode as the plural makes me think these are lists.


List<TransitStopVertex> stops = graph.getVerticesOfType(TransitStopVertex.class);
Set<StopLocation> carsAllowedStops = timetableRepository.getStopLocationsUsedForCarsAllowedTrips();

LOG.info("Creating transfers based on requests:");
transferRequests.forEach(transferProfile -> LOG.info(transferProfile.toString()));
if (transferParametersForMode.isEmpty()) {
LOG.info("No mode-specific transfer configurations provided.");
} else {
LOG.info("Using transfer configurations for modes:");
transferParametersForMode.forEach((mode, transferParameters) ->
LOG.info(mode + ": " + transferParameters)
);
}

ProgressTracker progress = ProgressTracker.track(
"Create transfer edges for stops",
Expand All @@ -90,16 +126,19 @@ public void buildGraph() {
HashMultimap.create()
);

List<RouteRequest> defaultTransferRequests = new ArrayList<>();
List<RouteRequest> carsAllowedStopTransferRequests = new ArrayList<>();
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()
);
}

// Parse the transfer configuration from the parameters given in the build config.
parseTransferParameters(
defaultTransferRequests,
carsAllowedStopTransferRequests,
flexTransferRequests,
defaultNearbyStopFinders,
carsAllowedStopNearbyStopFinders,
nearbyStopFinder
);
Comment on lines +133 to +141
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer this method to return a private record which contains the different transfer requests and stop finders instead of initializing them here and populating them inside this method, but we could discuss this in a dev meeting.

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 a dev meeting and agreed that we should return a record "result" from the method.


stops
.stream()
Expand All @@ -117,14 +156,11 @@ public void buildGraph() {
LOG.debug("Linking stop '{}' {}", stop, ts0);

// Calculate default transfers.
for (RouteRequest transferProfile : transferRequests) {
for (RouteRequest transferProfile : defaultTransferRequests) {
StreetMode mode = transferProfile.journey().transfer().mode();
for (NearbyStop sd : nearbyStopFinder.findNearbyStops(
ts0,
transferProfile,
transferProfile.journey().transfer(),
false
)) {
for (NearbyStop sd : defaultNearbyStopFinders
.get(mode)
.findNearbyStops(ts0, transferProfile, transferProfile.journey().transfer(), false)) {
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
continue;
Expand Down Expand Up @@ -152,12 +188,9 @@ public void buildGraph() {
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
)) {
for (NearbyStop sd : defaultNearbyStopFinders
.get(mode)
.findNearbyStops(ts0, transferProfile, transferProfile.journey().transfer(), true)) {
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
continue;
Expand All @@ -180,6 +213,39 @@ public void buildGraph() {
}
}
}
// Calculate transfers between stops that can use trips with cars if configured.
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
// Calculate transfers between stops that can use trips with cars if configured.
// Calculate transfers between stops that are visited by trips that allow cars, if configured.

if (carsAllowedStops.contains(stop)) {
for (RouteRequest transferProfile : carsAllowedStopTransferRequests) {
StreetMode mode = transferProfile.journey().transfer().mode();
for (NearbyStop sd : carsAllowedStopNearbyStopFinders
.get(mode)
.findNearbyStops(ts0, transferProfile, transferProfile.journey().transfer(), false)) {
Comment on lines +220 to +222
Copy link
Member

Choose a reason for hiding this comment

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

These for statements that span over multiple lines are difficult to read. Put the list in a variable and use that.

// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this block a copy&paste job from the one above? Can it be reused?

continue;
}
if (sd.stop.transfersNotAllowed()) {
continue;
}
// Only calculate transfers between carsAllowedStops.
if (!carsAllowedStops.contains(sd.stop)) {
continue;
}
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));
}
}
}
}

LOG.debug(
"Linked stop {} with {} transfers to stops with different patterns.",
Expand Down Expand Up @@ -227,7 +293,7 @@ public void buildGraph() {
* whether the graph has a street network and if ConsiderPatternsForDirectTransfers feature is
* enabled.
*/
private NearbyStopFinder createNearbyStopFinder() {
private NearbyStopFinder createNearbyStopFinder(Duration radiusByDuration) {
var transitService = new DefaultTransitService(timetableRepository);
NearbyStopFinder finder;
if (!graph.hasStreets) {
Expand All @@ -247,5 +313,55 @@ private NearbyStopFinder createNearbyStopFinder() {
}
}

private void parseTransferParameters(
List<RouteRequest> defaultTransferRequests,
List<RouteRequest> carsAllowedStopTransferRequests,
List<RouteRequest> flexTransferRequests,
HashMap<StreetMode, NearbyStopFinder> defaultNearbyStopFinders,
HashMap<StreetMode, NearbyStopFinder> carsAllowedStopNearbyStopFinders,
NearbyStopFinder nearbyStopFinder
) {
for (RouteRequest transferProfile : transferRequests) {
StreetMode mode = transferProfile.journey().transfer().mode();
TransferParameters transferParameters = transferParametersForMode.get(mode);
if (transferParameters != null) {
// Disable normal transfer calculations for the specific mode, if disableDefaultTransfers is set in the build config.
// WALK mode transfers can not be disabled. For example, flex transfers need them.
Comment on lines +328 to +329
Copy link
Member

Choose a reason for hiding this comment

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

Should we enforce this restriction with a warning/crash when reading in the configuration?

if (!transferParameters.disableDefaultTransfers() || mode == StreetMode.WALK) {
defaultTransferRequests.add(transferProfile);
// Set mode-specific maxTransferDuration, if it is set in the build config.
Duration maxTransferDuration = transferParameters.maxTransferDuration();
if (maxTransferDuration != Duration.ZERO) {
defaultNearbyStopFinders.put(mode, createNearbyStopFinder(maxTransferDuration));
} else {
defaultNearbyStopFinders.put(mode, nearbyStopFinder);
}
}
// Create transfers between carsAllowedStops for the specific mode if carsAllowedStopMaxTransferDuration is set in the build config.
Duration carsAllowedStopMaxTransferDuration = transferParameters.carsAllowedStopMaxTransferDuration();
if (carsAllowedStopMaxTransferDuration != Duration.ZERO) {
carsAllowedStopTransferRequests.add(transferProfile);
carsAllowedStopNearbyStopFinders.put(
mode,
createNearbyStopFinder(carsAllowedStopMaxTransferDuration)
);
}
} else {
defaultTransferRequests.add(transferProfile);
defaultNearbyStopFinders.put(mode, nearbyStopFinder);
}
}

// 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()
);
}
}

private record TransferKey(StopLocation source, StopLocation target, List<Edge> edges) {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package org.opentripplanner.graph_builder.module;

import java.time.Duration;
import org.opentripplanner.utils.tostring.ToStringBuilder;

public record TransferParameters(
Copy link
Member

Choose a reason for hiding this comment

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

In general we try to avoid exposing records in core code but in this context it might be ok.

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 a dev meeting and decided that using a record in this case is ok.

Duration maxTransferDuration,
Duration carsAllowedStopMaxTransferDuration,
boolean disableDefaultTransfers
) {
public static final Duration DEFAULT_MAX_TRANSFER_DURATION = Duration.ZERO;
public static final Duration DEFAULT_CARS_ALLOWED_STOP_MAX_TRANSFER_DURATION = Duration.ZERO;
public static final boolean DEFAULT_DISABLE_DEFAULT_TRANSFERS = false;

TransferParameters(Builder builder) {
this(
builder.maxTransferDuration,
builder.carsAllowedStopMaxTransferDuration,
builder.disableDefaultTransfers
);
}

public String toString() {
return ToStringBuilder
.of(getClass())
.addDuration("maxTransferDuration", maxTransferDuration)
.addDuration("carsAllowedStopMaxTransferDuration", carsAllowedStopMaxTransferDuration)
.addBool("disableDefaultTransfers", disableDefaultTransfers)
.toString();
}

public static class Builder {

private Duration maxTransferDuration;
private Duration carsAllowedStopMaxTransferDuration;
private boolean disableDefaultTransfers;

public Builder() {
this.maxTransferDuration = DEFAULT_MAX_TRANSFER_DURATION;
this.carsAllowedStopMaxTransferDuration = DEFAULT_CARS_ALLOWED_STOP_MAX_TRANSFER_DURATION;
this.disableDefaultTransfers = DEFAULT_DISABLE_DEFAULT_TRANSFERS;
}

public Builder withMaxTransferDuration(Duration maxTransferDuration) {
this.maxTransferDuration = maxTransferDuration;
return this;
}

public Builder withCarsAllowedStopMaxTransferDuration(
Duration carsAllowedStopMaxTransferDuration
) {
this.carsAllowedStopMaxTransferDuration = carsAllowedStopMaxTransferDuration;
return this;
}

public Builder withDisableDefaultTransfers(boolean disableDefaultTransfers) {
this.disableDefaultTransfers = disableDefaultTransfers;
return this;
}

public TransferParameters build() {
return new TransferParameters(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ static DirectTransferGenerator provideDirectTransferGenerator(
timetableRepository,
issueStore,
config.maxTransferDuration,
config.transferRequests
config.transferRequests,
config.transferParametersForMode
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.time.LocalDate;
import java.time.ZoneId;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
import javax.annotation.Nullable;
Expand All @@ -23,6 +24,7 @@
import org.opentripplanner.ext.emissions.EmissionsConfig;
import org.opentripplanner.ext.fares.FaresConfiguration;
import org.opentripplanner.framework.geometry.CompactElevationProfile;
import org.opentripplanner.graph_builder.module.TransferParameters;
import org.opentripplanner.graph_builder.module.ned.parameter.DemExtractParameters;
import org.opentripplanner.graph_builder.module.ned.parameter.DemExtractParametersList;
import org.opentripplanner.graph_builder.module.osm.parameters.OsmExtractParameters;
Expand All @@ -32,13 +34,16 @@
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.DemConfig;
import org.opentripplanner.standalone.config.buildconfig.GtfsConfig;
import org.opentripplanner.standalone.config.buildconfig.IslandPruningConfig;
import org.opentripplanner.standalone.config.buildconfig.NetexConfig;
import org.opentripplanner.standalone.config.buildconfig.OsmConfig;
import org.opentripplanner.standalone.config.buildconfig.S3BucketConfig;
import org.opentripplanner.standalone.config.buildconfig.TransferConfig;
import org.opentripplanner.standalone.config.buildconfig.TransferRequestConfig;
import org.opentripplanner.standalone.config.buildconfig.TransitFeedConfig;
import org.opentripplanner.standalone.config.buildconfig.TransitFeeds;
Expand Down Expand Up @@ -151,6 +156,7 @@ public class BuildConfig implements OtpDataStoreConfig {
public final IslandPruningConfig islandPruning;

public final Duration maxTransferDuration;
public final Map<StreetMode, TransferParameters> transferParametersForMode;
public final NetexFeedParameters netexDefaults;
public final GtfsFeedParameters gtfsDefaults;

Expand Down Expand Up @@ -287,6 +293,7 @@ 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));
transferParametersForMode = TransferConfig.map(root, "transfers");
maxStopToShapeSnapDistance =
root
.of("maxStopToShapeSnapDistance")
Expand Down
Loading
Loading