Restore lookup of multi-modal stations and group of stations #6805

leonardehrenfried · 2025-08-18T09:55:43Z

Summary

It restores the ability to use the id of a multi-modal station or a group of station for the start and end point of routing requests.

It does it by clarifying responsibilities:

TransitService resolves site ids into their child stops
StreetIndex only has references to stop vertices through their ID but doesn't know anything about stations, group of stations...
TemporaryVerticesContainer is the glue that takes the various data sources and returns the correct vertices.

Renaming

TemporaryVerticesContainer is not a good name anymore and I would like to rename it. Not sure if I should do it in this PR.

Issue

Fixes #6804

Unit tests

Lots added.

Documentation

Lots of methods clarified, Javadocs adde and method grouping improved in the various files.

codecov · 2025-08-18T10:02:09Z

Codecov Report

❌ Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.00%. Comparing base (3bae1a1) to head (fb3832a).
⚠️ Report is 369 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...planner/routing/graphfinder/StreetGraphFinder.java	0.00%	1 Missing ⚠️
...nner/street/search/TemporaryVerticesContainer.java	95.23%	0 Missing and 1 partial ⚠️
...rg/opentripplanner/visualizer/GraphVisualizer.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6805      +/-   ##
=============================================
+ Coverage      71.76%   72.00%   +0.24%     
- Complexity     19330    19437     +107     
=============================================
  Files           2090     2101      +11     
  Lines          78771    78774       +3     
  Branches        8027     7963      -64     
=============================================
+ Hits           56529    56724     +195     
+ Misses         19384    19242     -142     
+ Partials        2858     2808      -50

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

leonardehrenfried · 2025-08-18T10:27:46Z

@habrahamsson-skanetrafiken This one touches the station centroid feature and I'm not sure I've taken all the required subtleties into account. Can you test this with your data set, please?

habrahamsson-skanetrafiken

This looks fine to me. I'll verify the centroid stuff in our test environment.

There will be conflicts with #6704 i guess.

application/src/main/java/org/opentripplanner/street/search/TemporaryVerticesContainer.java

habrahamsson-skanetrafiken

These changes look good to me and I agree that the restructuring is a good one.

habrahamsson-skanetrafiken · 2025-08-19T10:38:09Z

application/src/main/java/org/opentripplanner/street/search/TemporaryVerticesContainer.java

   * return the child stops of that station.
   */
  public Set<TransitStopVertex> getFromStopVertices() {
    if (from.stopId == null) {


I had a look at the getFromVertices() and getFromStopVertices() methods etc. They look good to me. I understand that the naming is very similar and a bit confusing.

The thing I did with centroid coordinates is basically to be able to have separate vertices for the street search part and the transit search part.

So if you use a station with a centroid coordinate we will do the street search (and access/egress walking etc) using the centroid coordinate but still do the transit search using the child stop coordinates.

Do you think it would be easier to understand if we renamed the methods to getFromStreetVertices() and getFromStopOrChildStopVertices() perhaps?

I think it's fine the way it is. I was just wondering if the street vertex methods should return the centroid vertex or not. Sounds like they should not.

The stop vertex methods should not, but the street vertex methods should. And this PR seems to keep that behaviour so it looks fine.

application/src/main/java/org/opentripplanner/routing/graph/Graph.java

t2gran · 2025-08-26T12:40:41Z

application/src/main/java/org/opentripplanner/street/search/TemporaryVerticesContainer.java

+ * <p>
+ * It also facilitates the lookup of site ids to the correct vertices.
+ * <p>
+ * It implements


I created this class to take care of disposing temporary changes to the graph[v,e] . It is a framework "thing", and should not contain business rules (decide between centroid/coordinates/ids). I will approve this PR, since it is only patching the existing features - but this should be refactored, so this class get back to its original responsibility.

Where would you put this logic?

t2gran · 2025-08-26T12:49:55Z

application/src/main/java/org/opentripplanner/street/search/TemporaryVerticesContainer.java

+        // check if there is a stop by the given id
+        var stopVertex = graph.findStopVertex(location.stopId);
+        if (stopVertex.isPresent()) {
+          return Set.of(stopVertex.get());
+        }
+
+        // station centroids may be used instead of child stop vertices for stations
+        var centroidVertex = graph.findStationCentroidVertex(location.stopId);
+        if (centroidVertex.isPresent()) {
+          return Set.of(centroidVertex.get());
+        }
+
+        // in the regular case you want to resolve a (multi-modal) station into its child stops
+        var childVertices = findStopOrChildStopVertices(location.stopId);
+        if (!childVertices.isEmpty()) {
+          return childVertices
+            .stream()
+            .map(Vertex.class::cast)
+            .collect(Collectors.toUnmodifiableSet());
        }


Maybe this could be pushed into the Graph - at least the Graphs responsibilities matches better than this class.

The vertex part would be a good fit but should the Graph know about resolving stations to stops?

Also @habrahamsson-skanetrafiken might want to chime in because he asked me to move out of the graph to this class.

I think we should look at this together in the design meeting, with at least all three of us present.

I agree, but didn't you want something sooner so that you can release to prod?

I guess the question is if we want the Graph to know about the hierarchy between stops/stations/multi-modal stations etc. If we don't want the Graph to know about that stuff then this code needs to go somewhere else.

If we want to keep the TemporaryVerticesContainer small, maybe we could extract this id -> vertex resolving logic to a separate class that depends on both Graph and TransitService.

We can look at it in a design meeting.

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

…aph.java Co-authored-by: Thomas Gran <[email protected]>

application/src/test/java/org/opentripplanner/routing/core/TemporaryVerticesContainerTest.java

leonardehrenfried · 2025-09-08T08:42:56Z

I can work on this feature this week if there is a consensus what I should be doing.

t2gran · 2025-09-11T11:03:25Z

This is now blocking #6791 - there is a merge conflict. Can we merge this, and refactor in a separate PR?

t2gran

This PR should be followed up with a PR fixing the problem with the TemporaryVerticesContainer (discussed above). But, the bugfix is a blocker - so we want this to be merged.

leonardehrenfried · 2025-09-11T11:11:04Z

@habrahamsson-skanetrafiken already approved previously so I will merge this and refactor later.

leonardehrenfried added 7 commits August 18, 2025 11:35

Fix multi-modal linking

66804c9

Update test

16a003f

Clean up

ba14d3f

Reorder and rename methods

98aefe2

Clean up

454795f

Clarify responsibilities

ab67937

Add test for station lookup

085e065

leonardehrenfried requested a review from a team as a code owner August 18, 2025 09:55

leonardehrenfried requested a review from t2gran August 18, 2025 09:55

leonardehrenfried added the !Bug Apply to issues describing a bug and PRs witch fixes it. label Aug 18, 2025

leonardehrenfried changed the title ~~Fix lookup of multi-modal stations and group of stations~~ Restore lookup of multi-modal stations and group of stations Aug 18, 2025

optionsome requested review from habrahamsson-skanetrafiken and optionsome and removed request for optionsome August 18, 2025 10:11

leonardehrenfried added 2 commits August 18, 2025 12:28

Add test for index

40e53a1

Remove toString

641455b

leonardehrenfried mentioned this pull request Aug 18, 2025

Support for Multimodal Stops not working #6804

Closed

Simplify lookup

1d37651

habrahamsson-skanetrafiken requested changes Aug 18, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/street/search/TemporaryVerticesContainer.java Outdated Show resolved Hide resolved

Make methods in graph simpler

e17cac7

optionsome requested a review from habrahamsson-skanetrafiken August 19, 2025 10:01

habrahamsson-skanetrafiken reviewed Aug 19, 2025

View reviewed changes

t2gran added the Entur Test This is currently being tested at Entur label Aug 19, 2025

t2gran added this to the 2.8 (next release) milestone Aug 19, 2025

eibakke removed the Entur Test This is currently being tested at Entur label Aug 20, 2025

habrahamsson-skanetrafiken previously approved these changes Aug 21, 2025

View reviewed changes

t2gran requested changes Aug 26, 2025

View reviewed changes

Update application/src/main/java/org/opentripplanner/routing/graph/Gr…

3e4268e

…aph.java Co-authored-by: Thomas Gran <[email protected]>

leonardehrenfried dismissed habrahamsson-skanetrafiken’s stale review via 3e4268e August 26, 2025 13:11

Convert to List

1e029c6

t2gran reviewed Aug 26, 2025

View reviewed changes

application/src/test/java/org/opentripplanner/routing/core/TemporaryVerticesContainerTest.java Outdated Show resolved Hide resolved

Replace Set.of with List.of

fb3832a

t2gran added the Entur Test This is currently being tested at Entur label Aug 29, 2025

t2gran modified the milestones: 2.8, 2.9 (next release) Sep 10, 2025

t2gran approved these changes Sep 11, 2025

View reviewed changes

leonardehrenfried merged commit 1ea783b into opentripplanner:dev-2.x Sep 11, 2025
7 checks passed

leonardehrenfried deleted the fix-multimodal branch September 11, 2025 11:11

t2gran pushed a commit that referenced this pull request Sep 11, 2025

Add changelog entry for #6805 [ci skip]

d20bfc1

leonardehrenfried modified the milestones: 2.9 (next release), 2.8.1 Sep 22, 2025

Restore lookup of multi-modal stations and group of stations #6805

Restore lookup of multi-modal stations and group of stations #6805

Uh oh!

Conversation

leonardehrenfried commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Renaming

Issue

Unit tests

Documentation

Uh oh!

codecov bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leonardehrenfried commented Aug 18, 2025

Uh oh!

habrahamsson-skanetrafiken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

habrahamsson-skanetrafiken 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!

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!

Uh oh!

Uh oh!

leonardehrenfried commented Sep 8, 2025

Uh oh!

t2gran commented Sep 11, 2025

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leonardehrenfried commented Aug 18, 2025 •

edited

Loading

codecov bot commented Aug 18, 2025 •

edited

Loading