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.

Sign up for GitHub

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

Incrementally generate raptor transfers from constrained transfers for new pattern #6191

Draft
wants to merge 1 commit into
base: dev-2.x
Choose a base branch
from
+73 −17
Draft

Incrementally generate raptor transfers from constrained transfers for new pattern #6191

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
90 changes: 73 additions & 17 deletions ...r/routing/algorithm/raptoradapter/transit/constrainedtransfer/TransferIndexGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ public class TransferIndexGenerator {
private final Map<Route, Set<RoutingTripPattern>> patternsByRoute = new HashMap<>();
private final Map<Trip, Set<RoutingTripPattern>> patternsByTrip = new HashMap<>();

private int lastPatternIndex = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Rename to prevProcessedNumberOfPatterns, and make it equals to the size of the collection.

private TransferForPatternByStopPos[] prevForwardTransfers = {};
private TransferForPatternByStopPos[] prevReverseTransfers = {};
Comment on lines +46 to +47
Copy link
Member

Choose a reason for hiding this comment

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

The above code keep a reference to the list(array) of transfers kept elsewhere. This may lead to inconsistency problems, but I guess this is also the case for the other cashed data in this class. So I think we can ignore it for know. There is no performance gain of keeping these as arrays, so they can be ArrayLists. I will comment on the needed changes bellow to use ArrayList instead, the coping is also unnecessary, so:

  // We need to use ArrayList to be able to enforce the capacity
  private ArrayList<TransferForPatternByStopPos> forwardTransfers = new ArrayList<>();
  private ArrayList<TransferForPatternByStopPos> reverseTransfers = new ArrayList<>();


public TransferIndexGenerator(
Collection<ConstrainedTransfer> constrainedTransfers,
Collection<TripPattern> tripPatterns
Expand All @@ -52,9 +56,22 @@ public TransferIndexGenerator(

public ConstrainedTransfersForPatterns generateTransfers() {
int nPatterns = RoutingTripPattern.indexCounter();

// If no new patterns have been added, return the previously generated transfers
if (lastPatternIndex == nPatterns - 1) {
return new ConstrainedTransfersForPatterns(
Arrays.asList(prevForwardTransfers),
Arrays.asList(prevReverseTransfers)
);
Comment on lines +62 to +65
Copy link
Member

@t2gran t2gran Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not dry, make a private factory method, if using ArrayList it will be something like:

/**
 * TODO - Doc on: 
 *  - Why we set prevProcessedNumberOfPatterns here
 *  - Why we make a defensive copy of the list or push into ConstrainedTransfersForPatterns
 */
private ConstrainedTransfersForPatterns createConstrainedTransfersForPatterns() {
    this.prevProcessedNumberOfPatterns = prevForwardTransfers.size();
    return new ConstrainedTransfersForPatterns(
      // TODO: Push the copy into the construct instead of doing it here
      List.copyOf(forwardTransfers),
      List.copyOf(reverseTransfers)
    );
}

Note! No matter how you do this the arrays will be copied at this point, see Arrays.asList(), List.copyOf().

}

TransferForPatternByStopPos[] forwardTransfers = new TransferForPatternByStopPos[nPatterns];
TransferForPatternByStopPos[] reverseTransfers = new TransferForPatternByStopPos[nPatterns];

// Copy previously generated transfers
System.arraycopy(prevForwardTransfers, 0, forwardTransfers, 0, prevForwardTransfers.length);
System.arraycopy(prevReverseTransfers, 0, reverseTransfers, 0, prevReverseTransfers.length);

Comment on lines +71 to +74
Copy link
Member

Choose a reason for hiding this comment

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

With ArrayList this can be replaced with (line 68-73):

 // Ensure we increment the capacity once and not incremental when we add new transfers  
 forwardTransfers.ensureCapacity(nPatterns);
 reverseTransfers.ensureCapacity(nPatterns);

Unsure if this is needed, but at least it can hurt much (except readability).

for (ConstrainedTransfer tx : constrainedTransfers) {
var c = tx.getTransferConstraint();
// Only add transfers which have an effect on the Raptor routing here.
Expand All @@ -65,11 +82,13 @@ public ConstrainedTransfersForPatterns generateTransfers() {
}

try {
findTPoints(tx.getFrom(), ALIGHT)
findTPoints(tx.getFrom(), ALIGHT, false)
.stream()
.filter(TPoint::canAlight)
.forEachOrdered(fromPoint -> {
for (var toPoint : findTPoints(tx.getTo(), BOARD)) {
boolean alightFromPreviouslySeenPattern =
fromPoint.pattern.patternIndex() <= lastPatternIndex;
Comment on lines +89 to +90
Copy link
Member

Choose a reason for hiding this comment

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

The pattern.patternIndex() <= lastPatternIndex is a leaked abstraction, but it is difficult to fix here - I looked into it, but at least you should make sure it is dry - implemented in just one place with a private method:

private boolean `isNewPattern(RoutingTripPattern pattern) { return pattern.patternIndex() >= prevProcessedNumberOfPatterns; }`

and

boolean alightFromPreviouslySeenPattern = !isNewPattern(fromPoint.pattern);

for (var toPoint : findTPoints(tx.getTo(), BOARD, alightFromPreviouslySeenPattern)) {
if (toPoint.canBoard() && !fromPoint.equals(toPoint)) {
fromPoint.addTransferConstraints(tx, toPoint, forwardTransfers, reverseTransfers);
}
Expand All @@ -83,6 +102,11 @@ public ConstrainedTransfersForPatterns generateTransfers() {
sortTransfers(forwardTransfers);
sortTransfers(reverseTransfers);

// Update the last pattern handled index and store the generated transfers
lastPatternIndex = nPatterns - 1;
prevForwardTransfers = forwardTransfers;
prevReverseTransfers = reverseTransfers;

Comment on lines +105 to +109
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
// Update the last pattern handled index and store the generated transfers
lastPatternIndex = nPatterns - 1;
prevForwardTransfers = forwardTransfers;
prevReverseTransfers = reverseTransfers;

And replace the new Co... with the factory method.

return new ConstrainedTransfersForPatterns(
Arrays.asList(forwardTransfers),
Arrays.asList(reverseTransfers)
Expand Down Expand Up @@ -132,21 +156,25 @@ private void sortTransfers(TransferForPatternByStopPos[] transfers) {
}
}

private Collection<TPoint> findTPoints(TransferPoint txPoint, boolean boarding) {
private Collection<TPoint> findTPoints(
TransferPoint txPoint,
boolean boarding,
boolean onlyNewPatterns
) {
if (txPoint.isStationTransferPoint()) {
return findTPoints(txPoint.asStationTransferPoint());
return findTPoints(txPoint.asStationTransferPoint(), onlyNewPatterns);
} else if (txPoint.isStopTransferPoint()) {
return findTPoints(txPoint.asStopTransferPoint());
return findTPoints(txPoint.asStopTransferPoint(), onlyNewPatterns);
} else if (txPoint.isRouteStationTransferPoint()) {
return findTPoint(txPoint.asRouteStationTransferPoint(), boarding);
return findTPoint(txPoint.asRouteStationTransferPoint(), boarding, onlyNewPatterns);
} else if (txPoint.isRouteStopTransferPoint()) {
return findTPoint(txPoint.asRouteStopTransferPoint(), boarding);
return findTPoint(txPoint.asRouteStopTransferPoint(), boarding, onlyNewPatterns);
} else {
return findTPoints(txPoint.asTripTransferPoint());
return findTPoints(txPoint.asTripTransferPoint(), onlyNewPatterns);
}
}

private List<TPoint> findTPoints(StationTransferPoint point) {
private List<TPoint> findTPoints(StationTransferPoint point, boolean onlyNewPatterns) {
var station = point.getStation();
var patterns = patternsByStation.get(station);

Expand All @@ -158,6 +186,9 @@ private List<TPoint> findTPoints(StationTransferPoint point) {
var result = new ArrayList<TPoint>();

for (RoutingTripPattern pattern : patterns) {
if (onlyNewPatterns && pattern.patternIndex() <= lastPatternIndex) {
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
if (onlyNewPatterns && pattern.patternIndex() <= lastPatternIndex) {
if (onlyNewPatterns && !isNewPattern(pattern)) {

continue;
}
var tripPattern = pattern.getPattern();
for (int pos = 0; pos < tripPattern.numberOfStops(); ++pos) {
if (point.getStation() == tripPattern.getStop(pos).getParentStation()) {
Expand All @@ -168,7 +199,7 @@ private List<TPoint> findTPoints(StationTransferPoint point) {
return result;
}

private List<TPoint> findTPoints(StopTransferPoint point) {
private List<TPoint> findTPoints(StopTransferPoint point, boolean onlyNewPatterns) {
var stop = point.asStopTransferPoint().getStop();
var patterns = patternsByStop.get(stop);

Expand All @@ -180,6 +211,9 @@ private List<TPoint> findTPoints(StopTransferPoint point) {
var result = new ArrayList<TPoint>();

for (RoutingTripPattern pattern : patterns) {
if (onlyNewPatterns && pattern.patternIndex() <= lastPatternIndex) {
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
if (onlyNewPatterns && pattern.patternIndex() <= lastPatternIndex) {
if (onlyNewPatterns && isNewPattern(pattern)) {

continue;
}
var p = pattern.getPattern();
for (int pos = 0; pos < p.numberOfStops(); ++pos) {
if (point.getStop() == p.getStop(pos)) {
Expand All @@ -190,27 +224,38 @@ private List<TPoint> findTPoints(StopTransferPoint point) {
return result;
}

private List<TPoint> findTPoint(RouteStationTransferPoint point, boolean boarding) {
private List<TPoint> findTPoint(
RouteStationTransferPoint point,
boolean boarding,
boolean onlyNewPatterns
) {
return findTPointForRoute(
point.getRoute(),
boarding
? p -> p.findBoardingStopPositionInPattern(point.getStation())
: p -> p.findAlightStopPositionInPattern(point.getStation())
: p -> p.findAlightStopPositionInPattern(point.getStation()),
onlyNewPatterns
);
}

private List<TPoint> findTPoint(RouteStopTransferPoint point, boolean boarding) {
private List<TPoint> findTPoint(
RouteStopTransferPoint point,
boolean boarding,
boolean onlyNewPatterns
) {
return findTPointForRoute(
point.getRoute(),
boarding
? p -> p.findBoardingStopPositionInPattern(point.getStop())
: p -> p.findAlightStopPositionInPattern(point.getStop())
: p -> p.findAlightStopPositionInPattern(point.getStop()),
onlyNewPatterns
);
}

private List<TPoint> findTPointForRoute(
Route route,
ToIntFunction<TripPattern> resolveStopPosInPattern
ToIntFunction<TripPattern> resolveStopPosInPattern,
boolean onlyNewPatterns
) {
var patterns = patternsByRoute.get(route);

Expand All @@ -222,6 +267,9 @@ private List<TPoint> findTPointForRoute(
var points = new ArrayList<TPoint>();

for (var pattern : patterns) {
if (onlyNewPatterns && pattern.patternIndex() <= lastPatternIndex) {
continue;
}
int stopPosInPattern = resolveStopPosInPattern.applyAsInt(pattern.getPattern());
// stopPosInPattern == -1 means stop is not on pattern
if (stopPosInPattern >= 0) {
Expand All @@ -233,7 +281,7 @@ private List<TPoint> findTPointForRoute(
return points;
}

private List<TPoint> findTPoints(TripTransferPoint point) {
private List<TPoint> findTPoints(TripTransferPoint point, boolean onlyNewPatterns) {
var trip = point.getTrip();

// All trips have at least one pattern, no need to check for null here
Expand Down Expand Up @@ -263,15 +311,23 @@ private List<TPoint> findTPoints(TripTransferPoint point) {
// Return early if only scheduled pattern exists
var realtimePatterns = patternsByRealtimeOrScheduled.get(Boolean.TRUE);
if (realtimePatterns == null || realtimePatterns.isEmpty()) {
if (onlyNewPatterns && scheduledPattern.patternIndex() <= lastPatternIndex) {
return List.of();
}
return List.of(scheduledPoint);
}

// Process the other patterns based on the scheduled pattern
StopLocation scheduledStop = scheduledPattern.getPattern().getStop(stopPosInPattern);

List<TPoint> res = new ArrayList<>();
res.add(scheduledPoint);
if (!onlyNewPatterns || scheduledPattern.patternIndex() > lastPatternIndex) {
res.add(scheduledPoint);
}
for (RoutingTripPattern pattern : realtimePatterns) {
if (onlyNewPatterns && pattern.patternIndex() <= lastPatternIndex) {
continue;
}
// Check if the same stop or its sibling is at the same position, if yes, generate a transfer point
if (stopPosInPattern < pattern.numberOfStopsInPattern()) {
StopLocation stop = pattern.getPattern().getStop(stopPosInPattern);
Expand Down
Loading