Enable mode-specific transfers by storing mode information in transfers #6293

VillePihlava · 2024-11-29T13:57:05Z

Summary

This PR adds StreetMode information to transfers. This enables using mode-specific transfers during runtime.

Notes:

Instead of a collection of PathTransfers with mode information, this could also be done by using mappings from modes to transfers.
- Depending on how this is done, this could maybe be cheaper in terms of memory?
- In my tests the size of the graph has a similar size with both implementations. I prefer the implementation in this PR
The size of the graph can increase a lot when using a large dataset and when using CAR transfers.
- When I tested building the graph with data from almost the entirety of Finland, the size of the graph file increased from 1.4G to 6.9G.
- When using car transfers, because the transfers are calculated based on a range by duration there can be a lot of transfers.
- The solution is to use CAR transfers only with relevant stops.
- I will create a PR in the future for optionally limiting which stops can use transfers with cars.
- Using an EnumSet was the cheapest way I could think of adding this functionality when having the information in PathTransfer.
For large datasets with many transfers, build duration can increase.
- When I tested building the graph with data from almost the entirety of Finland, build duration doubled.
- For less transfers, build duration does not increase as much.

Issue

Resolves a TODO comment about adding StreetMode information in the PathTransfer class.

Related to #5875

Unit tests

Added a new test to DirectTransferGeneratorTest.

Documentation

No documentation has been added.

Bumping the serialization version id

This changes how transfers are generated during the graph build.

…ic transfers.

VillePihlava · 2024-11-29T13:59:15Z

The tests will fail for CarPickupSnapshotTest. There is discussion related to changing the test in #6238. This PR should not be merged before that test has been changed and the tests pass.

VillePihlava · 2024-11-29T14:08:38Z

Interestingly FlexIntegrationTest fails as well. I was able to locally run this test and all other tests except CarPickupSnapshotTest successfully, but I have noticed some flakiness related to running that test and running tests in general.

VillePihlava · 2024-11-29T14:33:13Z

Interestingly now after making changes not related to functionality, the FlexIntegrationTest did not fail.

…into split-transfers-by-mode-pathtransfer-mode

…er requests for WALK mode.

codecov · 2024-12-04T08:49:20Z

Codecov Report

Attention: Patch coverage is 78.20513% with 17 lines in your changes missing coverage. Please review.

Project coverage is 69.81%. Comparing base (69e6f64) to head (95e9ca7).
Report is 101 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...apping/StreetModeToTransferTraverseModeMapper.java	14.28%	5 Missing and 1 partial ⚠️
...pentripplanner/api/parameter/QualifiedModeSet.java	0.00%	3 Missing ⚠️
.../graph_builder/module/DirectTransferGenerator.java	91.17%	1 Missing and 2 partials ⚠️
...raptoradapter/transit/mappers/TransfersMapper.java	71.42%	2 Missing ⚠️
...t/java/org/opentripplanner/ext/flex/FlexIndex.java	75.00%	1 Missing ⚠️
.../java/org/opentripplanner/ext/flex/FlexRouter.java	0.00%	1 Missing ⚠️
...n/java/org/opentripplanner/model/PathTransfer.java	87.50%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6293      +/-   ##
=============================================
+ Coverage      69.79%   69.81%   +0.02%     
- Complexity     17788    17836      +48     
=============================================
  Files           2017     2020       +3     
  Lines          76042    76297     +255     
  Branches        7781     7804      +23     
=============================================
+ Hits           53074    53270     +196     
- Misses         20264    20311      +47     
- Partials        2704     2716      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

optionsome · 2024-12-05T12:23:03Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+            if (pathTransfer == null) {
+              EnumSet<StreetMode> modes = EnumSet.of(mode);
+              distinctTransfers.put(
+                transferKey,
+                new PathTransfer(stop, sd.stop, sd.distance, sd.edges, modes)
+              );
+            } else {
+              pathTransfer.addMode(mode);
+            }


You could add a comment here to explain what these different cases mean.

I added comments for the different cases

optionsome · 2024-12-05T12:29:17Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+        }
+        // Calculate flex transfers if flex routing is enabled.
+        for (RouteRequest transferProfile : flexTransferRequests) {
+          StreetMode mode = transferProfile.journey().transfer().mode();


Is this just always WALK?

We have always assumed that flex transfers can only be on foot. I think this is reasonable.

I changed this to StreetMode mode = StreetMode.WALK;

optionsome · 2024-12-05T12:41:22Z

...tion/src/test/java/org/opentripplanner/graph_builder/module/DirectTransferGeneratorTest.java

@@ -250,6 +250,61 @@ public void testMultipleRequestsWithPatterns() {
    );
  }

+  @Test
+  public void testPathTransfersWithModesForMultipleRequestsWithPatterns() {


What does multiple requests with patterns mean?

I tried to follow a similar naming convention as the other tests in the file. With patterns means that trippatterns are added to the timetablerepository of the test. Multiple requests means that transfers are generated for multiple requests that have different modes:

var reqWalk = new RouteRequest(); reqWalk.journey().transfer().setMode(StreetMode.WALK); var reqBike = new RouteRequest(); reqBike.journey().transfer().setMode(StreetMode.BIKE); var transferRequests = List.of(reqWalk, reqBike);

leonardehrenfried · 2024-12-06T12:31:40Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

    this.toStop = toStop;
    this.edges = edges;
    this.distanceMeters = (int) edges.stream().mapToDouble(Edge::getDistanceMeters).sum();
+    this.modes = modes;


We probably want to make this immutable. EnumSet are not.

I changed the getModes method to clone the enum set

leonardehrenfried · 2024-12-06T12:32:30Z

...tion/src/test/java/org/opentripplanner/graph_builder/module/DirectTransferGeneratorTest.java

+    var walkTransfers = timetableRepository
+      .getAllPathTransfers()
+      .stream()
+      .filter(pathTransfer -> pathTransfer.getModes().contains(StreetMode.WALK))
+      .toList();
+    var bikeTransfers = timetableRepository
+      .getAllPathTransfers()
+      .stream()
+      .filter(pathTransfer -> pathTransfer.getModes().contains(StreetMode.BIKE))
+      .toList();
+    var carTransfers = timetableRepository
+      .getAllPathTransfers()
+      .stream()
+      .filter(pathTransfer -> pathTransfer.getModes().contains(StreetMode.CAR))
+      .toList();


please extract a method

I created a getTransfersByMode method in TimetableRepository

leonardehrenfried · 2024-12-06T12:36:05Z

The solution is to use CAR transfers only with relevant stops.

Does this mean that this PR allows you to compute CAR transfers between stops that don't serve cars at all?

leonardehrenfried · 2024-12-06T12:39:50Z

...in/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/RaptorTransferIndex.java

@@ -41,6 +43,7 @@ public static RaptorTransferIndex create(
      var transfers = transfersByStopIndex
        .get(fromStop)
        .stream()
+        .filter(transfer -> transfer.getModes().contains(mode))


This is run only once and then cached for the specific combination of request parameters, correct?

Yes, this is run once for each cache request. You can see the code that uses this in RaptorRequestTransferCache

leonardehrenfried · 2024-12-06T12:50:17Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

+    return this.modes;
+  }
+
+  public boolean addMode(StreetMode mode) {


I don't think we want to make this class mutable. Can you create a copy method, for example withAddedMode that returns a new instance instead?

I changed this to use the withAddedMode method

leonardehrenfried · 2024-12-06T12:51:02Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

@@ -46,6 +56,14 @@ public List<Edge> getEdges() {
    return this.edges;
  }

+  public EnumSet<StreetMode> getModes() {


We don't want to expose the mutable EnumSet. Can you replace it with a method like allows(CAR)?

I changed the getModes method to clone the enum set

leonardehrenfried · 2024-12-06T12:58:19Z

application/src/ext/java/org/opentripplanner/ext/flex/FlexIndex.java

      transfersToStop.put(transfer.to, transfer);
+      transfersFromStop.put(transfer.from, transfer);


Can you explain why this is now necessary?

I think this is not strictly necessary, but instead it limits the available transfers to the WALK mode when searching for fromstops for flex transfers. Previously all of the transfers for a stop, including non-walk mode transfers, were given when using the getTransfersFromStop method in the FlexRouter. This was because it used the findPathTransfers method which gives all transfers for a given stop.

I think this could use my current implementation or the old one. I do have a somewhat shallow understanding of flex routing at the moment, but I went with this implementation because it seems more correct.

leonardehrenfried · 2024-12-06T13:03:44Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+      .map(transferProfile -> transferProfile.journey().transfer().mode())
+      .collect(Collectors.toSet())) {


Suggested change

.map(transferProfile -> transferProfile.journey().transfer().mode())

.collect(Collectors.toSet())) {

.map(transferProfile -> transferProfile.journey().transfer().mode())

.distinct()

.toList()

Not a huge deal but avoiding a set gives you predictable sort order.

Thanks, this is a good suggestion. I changed it to this

VillePihlava · 2024-12-09T11:58:40Z

Does this mean that this PR allows you to compute CAR transfers between stops that don't serve cars at all?

Yes, in a future PR (#6215) I will implement more configurability for transfers

leonardehrenfried · 2024-12-09T12:03:51Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

+  }
+
+  public EnumSet<StreetMode> getModes() {
+    return modes.clone();


Suggested change

return modes.clone();

return EnumSet.copyOf(modes);

I changed this

leonardehrenfried · 2024-12-09T12:08:58Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

    this.toStop = toStopIndex;
    this.distanceMeters = distanceMeters;
    this.edges = null;
+    this.modes = modes;


This transfer is not serialized. Here you can use Sets.immutableEnumSet and then never have to worry about cloning or copying.

I changed this to use Sets.immutableEnumSet

optionsome · 2024-12-10T14:33:53Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+    for (StreetMode mode : transferRequests
+      .stream()
+      .map(transferProfile -> transferProfile.journey().transfer().mode())
+      .distinct()
+      .toList()) {
+      LOG.info(
+        "Created {} transfers for mode {}.",
+        transfersByStop
+          .values()
+          .stream()
+          .filter(pathTransfer -> pathTransfer.getModes().contains(mode))
+          .count(),
+        mode
+      );


This is quite difficult to read. I would probably continue the stream instead of making it a list and then looping over it, and then call some method with the mode as an argument that does the logging. So something like:

transferRequests .stream() .map(transferProfile -> transferProfile.journey().transfer().mode()) .distinct() .peek(mode -> logTransfersForMode(mode));

I changed it to this:

transferRequests .stream() .map(transferProfile -> transferProfile.journey().transfer().mode()) .distinct() .forEach(mode -> LOG.info( "Created {} transfers for mode {}.", timetableRepository.getTransfersByMode(mode).size(), mode ) );

miklcct · 2024-12-10T16:24:22Z

I am following this as I am now testing BICYCLE routing on our deployment. The current deployment configuration only has "walk" with PT15M in build-config.json and the bicycle routing is near-useless for cross-London journeys, as it can't return results cycling from one London terminal to another (some of these transfers require about 30 minutes on the bike).

However, if I set the build-config.json to do walk and bike transfers with PT30M duration, the graph has become 15.1 GB just for London only without any other parts of Great Britain, and the transfer cache takes forever to build rather than seconds that it can't even cope with a city-sized deployment, and I can't even think of how it can work when put on a country-wide deployment.

VillePihlava · 2024-12-10T17:29:44Z

I am following this as I am now testing BICYCLE routing on our deployment. The current deployment configuration only has "walk" with PT15M in build-config.json and the bicycle routing is near-useless for cross-London journeys, as it can't return results cycling from one London terminal to another (some of these transfers require about 30 minutes on the bike).

However, if I set the build-config.json to do walk and bike transfers with PT30M duration, the graph has become 15.1 GB just for London only without any other parts of Great Britain, and the transfer cache takes forever to build rather than seconds that it can't even cope with a city-sized deployment, and I can't even think of how it can work when put on a country-wide deployment.

Without knowing your deployment in more detail, this sounds like there is something wrong with your configuration. While this PR limits the transfers to a specific mode thus maybe improving performance, the main reason for implementing this PR was to remove weird transfers that were relevant for a certain mode but not necessarily for another.

Therefore I suggest looking more closely at your configuration for performance improvements. A large graph size because of transfers could be caused by, for example, lots of transfers or transfers with lots of edges.

miklcct · 2024-12-10T17:35:48Z

How many transfers are in your graph? If I enable 45-minute transfers on bikes, it will generate approximately 15 million transfers just in London (as there are about 44k stops in London - however only thousands will actually serve bikes)! However, the majority of these transfers are useless and only those stops which have bike permitting services (a minority of the stops) are relevant, and without change, the graph is so large that it is flooded with useless transfers to the extent that it is unusable.

Long transfers are needed to enable people cycling across London to connect between intercity trains when local trains do not run between London terminals, or between pairs of terminals where no local trains run.

VillePihlava · 2024-12-10T17:44:22Z

I do not remember the amount of transfers off the top of my head. However, as I have written in this PR the graph for the entirety of Finland is ~1.4gb (not the exact number but in the same ballpark) and when the transfers were badly configured with the CAR mode, the same graph was ~6.9gb.

miklcct · 2024-12-10T17:48:48Z

I do not remember the amount of transfers off the top of my head. However, as I have written in this PR the graph for the entirety of Finland is ~1.4gb (not the exact number but in the same ballpark) and when the transfers were badly configured with the CAR mode, the same graph was ~6.9gb.

A graph with 45 minutes walk and bicycle transfers is 15 GB just for Greater London.

I am afraid it may grow to hundreds of GB if I attempt to pull this off in Hong Kong (where the stop density is multiple times higher than London).

optionsome · 2024-12-12T11:14:49Z

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

@@ -434,7 +435,7 @@ public Collection<PathTransfer> getTransfersByStop(StopLocation stop) {
    return transfersByStop.get(stop);
  }

-  public Collection<PathTransfer> getTransfersByMode(StreetMode mode) {
+  public List<PathTransfer> getTransfersByMode(StreetMode mode) {


I think this method should be called findTransfers according to the naming conventions.

I renamed getTransfersByMode to findTransfers

leonardehrenfried · 2024-12-12T12:24:45Z

@t2gran can we run the speed tests against that? Do you know how to do it?

VillePihlava added 4 commits November 29, 2024 15:25

Rename 'transferByStopIndex' variable to 'transfersByStopIndex'.

33189c2

Add changes for CAR transfers.

aa4927e

Add EnumSet of StreetModes to PathTransfers and implement mode-specif…

4c882f4

…ic transfers.

Add test for mode-specific transfers.

44e4d31

VillePihlava requested a review from a team as a code owner November 29, 2024 13:57

Remove unused imports and small changes.

8448e6e

Add logging for mode-specific transfers.

3c46be3

habrahamsson-skanetrafiken requested a review from optionsome December 3, 2024 10:30

VillePihlava added 2 commits December 4, 2024 07:14

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

c4951ff

…into split-transfers-by-mode-pathtransfer-mode

Fix flex routing transfer calculations and only calculate flex transf…

244c537

…er requests for WALK mode.

optionsome reviewed Dec 5, 2024

View reviewed changes

leonardehrenfried reviewed Dec 6, 2024

View reviewed changes

Add changes based on review comments.

012f149

leonardehrenfried reviewed Dec 9, 2024

View reviewed changes

Change how EnumSet is copied and make EnumSet immutable in Transfer.

984d34e

optionsome reviewed Dec 10, 2024

View reviewed changes

Change logging in transfer calculations.

3ae6eed

VillePihlava force-pushed the split-transfers-by-mode-pathtransfer-mode branch from a68ad75 to 3ae6eed Compare December 11, 2024 14:44

VillePihlava requested review from optionsome and leonardehrenfried December 11, 2024 14:51

Refactor to use method.

4a01328

optionsome reviewed Dec 12, 2024

View reviewed changes

Rename getTransfersByMode to findTransfers.

95e9ca7

optionsome approved these changes Dec 12, 2024

View reviewed changes

		transfersToStop.put(transfer.to, transfer);
		transfersFromStop.put(transfer.from, transfer);

		.map(transferProfile -> transferProfile.journey().transfer().mode())
		.collect(Collectors.toSet())) {

Enable mode-specific transfers by storing mode information in transfers #6293

Are you sure you want to change the base?

Enable mode-specific transfers by storing mode information in transfers #6293

Conversation

VillePihlava commented Nov 29, 2024 • edited Loading

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

VillePihlava commented Nov 29, 2024

VillePihlava commented Nov 29, 2024

VillePihlava commented Nov 29, 2024

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

optionsome Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miklcct commented Dec 10, 2024

VillePihlava commented Dec 10, 2024

miklcct commented Dec 10, 2024 • edited Loading

VillePihlava commented Dec 10, 2024

miklcct commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Dec 12, 2024

VillePihlava commented Nov 29, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

VillePihlava Dec 9, 2024 •

edited

Loading

optionsome Dec 10, 2024 •

edited

Loading

miklcct commented Dec 10, 2024 •

edited

Loading