-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: dev-2.x
Are you sure you want to change the base?
Changes from all commits
36336bf
f1380e3
2175e64
6418e26
e1e9bbb
89e617f
a1e0807
21bb307
573078a
3fd5bf5
35c98bc
3a34aa4
97238a3
41a0674
1a3f846
2e048f9
c90c3ec
30cc925
2e57c2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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; | ||||||
|
@@ -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; | ||||||
|
@@ -56,14 +60,31 @@ public DirectTransferGenerator( | |||||
Graph graph, | ||||||
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(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
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 | ||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would call these |
||||||
|
||||||
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", | ||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||||||
|
@@ -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; | ||||||
|
@@ -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; | ||||||
|
@@ -180,6 +213,39 @@ public void buildGraph() { | |||||
} | ||||||
} | ||||||
} | ||||||
// Calculate transfers between stops that can use trips with cars if configured. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.", | ||||||
|
@@ -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) { | ||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?