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

Open
wants to merge 26 commits into
base: dev-2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 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
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?

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 added a short 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();

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 to the suggestion.

}

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<>();
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to the suggestion.


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
);
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.

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 now returns a record.


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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to the suggestion.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to the suggestion.

// 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?

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 created a method for creating the PathTransfer and TransferKey. Is this fine?

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.
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?

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 throw an IllegalArgumentException when WALK mode transfers are configured to be disabled. Should a check also be made for a transferRequests field without the WALK mode?

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(
VillePihlava marked this conversation as resolved.
Show resolved Hide resolved
Duration maxTransferDuration,
Duration carsAllowedStopMaxTransferDuration,
boolean disableDefaultTransfers
) {
public static final Duration DEFAULT_MAX_TRANSFER_DURATION = Duration.ZERO;
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 the default 30 minutes? I think the default should be set only in one place and then used from there elsewhere.

public static final Duration DEFAULT_CARS_ALLOWED_STOP_MAX_TRANSFER_DURATION = Duration.ZERO;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if the default value for this should be the same as the normal default or null.

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