Access egress carpooling implementation by hakoncarlsen · Pull Request #7428 · opentripplanner/OpenTripPlanner

hakoncarlsen · 2026-03-16T14:02:54Z

Summary

Implementation of the function routeAccessEgress in DefaultCarpoolingService.

Issue

Notes:

Logic for creating and fetching vertices from the LinkingContext is refractored into a new class StreetVertexUtils.
A new router class CarpoolTreeStreetRouter is created for efficient one to many routing, which is necessary to make access/egress fast enough.
Changed DEFAULT_AVAILABLE_SEATS in CarpoolSiriMapper from 2 to 5. A strictly speaking unrelated change, but the small capacity of 2 made testing difficult. The way capacity is handled will in any case be changed in a future PR.

Closes #45

Unit tests

Write a few words on how the new code is tested.

Were unit tests added/updated?
There are added unit tests for the new router CarpoolTreeStreetRouter and the utility class StreetVertexUtils, and integration tests for both the routeDirect and routeAccessEgress functions of DefaultCarpoolingservice.
Was any manual verification done?
Yes, both routeDirect and routeAccessEgress works when running OTP locally.

Documentation

Have you added documentation in code covering design and rationale behind the code?
Yes
Were all non-trivial public classes and methods documented with Javadoc?
Yes
Were any new configuration options added? If so were the tables in
No

codecov · 2026-03-16T14:09:50Z

Codecov Report

❌ Patch coverage is 89.47368% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.32%. Comparing base (becd97c) to head (a5c9dcf).
⚠️ Report is 161 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...t/carpooling/service/DefaultCarpoolingService.java	92.06%	5 Missing and 5 partials ⚠️
...ner/ext/carpooling/routing/InsertionEvaluator.java	83.63%	7 Missing and 2 partials ⚠️
...xt/carpooling/routing/CarpoolTripWithVertices.java	63.15%	5 Missing and 2 partials ⚠️
...pooling/filter/DirectionalCompatibilityFilter.java	81.81%	0 Missing and 2 partials ⚠️
...xt/carpooling/routing/CarpoolTreeStreetRouter.java	97.01%	1 Missing and 1 partial ⚠️
...ner/ext/carpooling/filter/DistanceBasedFilter.java	83.33%	0 Missing and 1 partial ⚠️
...planner/ext/carpooling/filter/TimeBasedFilter.java	80.00%	0 Missing and 1 partial ⚠️
...ntripplanner/ext/carpooling/filter/TripFilter.java	0.00%	1 Missing ⚠️
...planner/ext/carpooling/util/StreetVertexUtils.java	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #7428      +/-   ##
=============================================
- Coverage      71.36%   71.32%   -0.05%     
- Complexity     21067    21135      +68     
=============================================
  Files           2343     2357      +14     
  Lines          87055    87528     +473     
  Branches        8612     8663      +51     
=============================================
+ Hits           62128    62426     +298     
- Misses         21922    22090     +168     
- Partials        3005     3012       +7

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…etRouter in carpooling module, and implemented access/egress for carpooling

arnljot

Mostly just camel case mistakes. Two of my comments are not request for changes but rather points if it should be improved or added.

arnljot · 2026-03-26T09:58:45Z

+            request.preferences().car().reluctance()
+          )
+        )
+        .toList();


We should consider if this also should be limited like routeDirect()

.limit(DEFAULT_MAX_CARPOOL_DIRECT_RESULTS)

This is related to the following conversation in the PR for the core changes that enabled this PR:

#7341 (comment)

I gave the following answer:

As for stopCountLimit, this might be relevant for the computational complexity in later steps of the pipeline, since it limits the input size which the raptor algorithm has to work through. If too many access/egress results can cause problems there, we should include this variable. Note: it will cause loosing relevant results especially if we want the option of having very long carpooling access/egresses.

I say we don't put a limit of the number of results for now, but we might have to in the future.

arnljot · 2026-03-26T10:09:22Z

+    try (var temporaryVerticesContainer = new TemporaryVerticesContainer()) {
+      var streetVertexUtils = new StreetVertexUtils(this.vertexLinker, temporaryVerticesContainer);
+
+      var carPoolTreeVertexRouter = new CarpoolTreeStreetRouter();


typo:
var carPoolTreeVertexRouter = new CarpoolTreeStreetRouter();
should be:
var carpoolTreeVertexRouter = new CarpoolTreeStreetRouter();

changed as suggested

arnljot · 2026-03-26T10:10:41Z

+        0
+      );
+
+      var nearByStops = streetNearbyStopFinder


typo, should be
var nearbyStops

changed as suggested

arnljot · 2026-03-26T10:11:12Z

+        .filter(stop -> !(stop.stop instanceof AreaStop))
+        .toList();
+
+      var nearByStopsWithVertices = new HashMap<NearbyStop, Vertex>();


same typo here
var nearbyStopsWithVertices

changed as suggested

arnljot · 2026-03-26T10:20:31Z

+        .filter(Objects::nonNull)
+        .toList();
+
+      // vertices have to be added to the carPoolTreeVertexRouter AFTER all vertices have been created


Lets consider if this two phase setup can be incapsulated inside the router or StreetVertexUtils. As now this limitation is only communicated in this comment. If it can't be incapsulated in the router or the util, then perhaps it can be grouped in it's own private method?

As it stands this constraint is easy to violate in future edits, however the code is well covered with tests so it might be left to this comment and current test coverage.

This should no longer be an issue now. I have done the following changes:

We no longer create temporary vertices for NearbyStops. This seemed to be necessary earlier, but everything works now by simply fetching the stop's vertex like this: stop.state.getVertex() .

CarpoolTreeStreetRouter lazily creates SPTs and you can no longer add vertices after routing has started. This is both good for performance, and it makes sure that you cannot create SPTs which don't include all temporary vertices.

arnljot · 2026-03-26T10:26:08Z

  private static final GeometryFactory GEOMETRY_FACTORY = new GeometryFactory();

-  private static final int DEFAULT_AVAILABLE_SEATS = 2;
+  private static final int DEFAULT_AVAILABLE_SEATS = 5;


Mention this in PR description, it's unrelated to the access egress change, but good and too trivial to address in a separate change.

I have added a comment about it in the PR description

arnljot · 2026-03-26T12:18:38Z

-    GenericLocation to,
-    LinkingContext linkingContext
-  );
+public interface CarpoolRouter {


Very smooth change, good domain oriented improvement

Thank you 😄

arnljot · 2026-03-26T12:21:29Z

@eibakke and @hakoncarlsen - I've looked over the change and it looks good to me, nothing needs change except the camel case mistakes. Then the two other comments can be disregarded if Håkon chooses to.

…ating temporary vertices for nearbyStops in routeAccessEgress in DefaultCarpoolingService

eibakke

Mostly looks good, just a comment about the filter interface duplication.

… class TripFilter instead

leonardehrenfried

This PR adheres to the sandbox contract.

hakoncarlsen requested a review from a team as a code owner March 16, 2026 14:02

hakoncarlsen marked this pull request as draft March 16, 2026 14:03

hakoncarlsen mentioned this pull request Mar 16, 2026

Added carpooling as an access/egress mode #7341

Merged

t2gran added this to the 2.10 (next release) milestone Mar 18, 2026

hakoncarlsen force-pushed the access_egress_carpooling_implementation branch 10 times, most recently from fc2af08 to 31e0acd Compare March 23, 2026 12:06

hakoncarlsen marked this pull request as ready for review March 23, 2026 12:14

added utility class StreetVertexUtils and new router CarpoolTreeStre…

3efa617

…etRouter in carpooling module, and implemented access/egress for carpooling

hakoncarlsen force-pushed the access_egress_carpooling_implementation branch from 31e0acd to 3efa617 Compare March 23, 2026 16:52

optionsome assigned leonardehrenfried and eibakke Mar 24, 2026

optionsome requested review from eibakke and leonardehrenfried March 24, 2026 09:05

optionsome added the +Sandbox This will be implemented as a Sandbox feature label Mar 24, 2026

leonardehrenfried added the +Skip Changelog This is not a relevant change for a product owner since last release. label Mar 26, 2026

arnljot reviewed Mar 26, 2026

View reviewed changes

fixed typos in DefaultCarpoolingService

ad0b705

hakoncarlsen force-pushed the access_egress_carpooling_implementation branch from 5c0e89e to 0e45178 Compare March 27, 2026 09:17

changed CarpoolTreeStreetRouter to lazily compute SPTs, no longer cre…

6f06598

…ating temporary vertices for nearbyStops in routeAccessEgress in DefaultCarpoolingService

hakoncarlsen force-pushed the access_egress_carpooling_implementation branch from 0e45178 to 6f06598 Compare March 27, 2026 09:25

eibakke requested changes Apr 9, 2026

View reviewed changes

Comment thread application/src/ext/java/org/opentripplanner/ext/carpooling/filter/AccessEgressTripFilter.java Outdated

removed AccessEgressTripFilter, added function acceptsAccessEgress to…

a5c9dcf

… class TripFilter instead

eibakke self-requested a review April 9, 2026 13:03

eibakke approved these changes Apr 9, 2026

View reviewed changes

habrahamsson-skanetrafiken requested a review from jessicaKoehnke April 9, 2026 13:30

habrahamsson-skanetrafiken assigned jessicaKoehnke and unassigned leonardehrenfried Apr 9, 2026

leonardehrenfried approved these changes Apr 9, 2026

View reviewed changes

sigtot added this pull request to the merge queue Apr 10, 2026

Merged via the queue into opentripplanner:dev-2.x with commit 92b481d Apr 10, 2026
8 checks passed

sigtot deleted the access_egress_carpooling_implementation branch April 10, 2026 07:56

Conversation

hakoncarlsen commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Uh oh!

codecov bot commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arnljot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hakoncarlsen Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnljot commented Mar 26, 2026

Uh oh!

eibakke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hakoncarlsen commented Mar 16, 2026 •

edited

Loading

codecov bot commented Mar 16, 2026 •

edited

Loading

hakoncarlsen Mar 26, 2026 •

edited

Loading