Support for routing to Station centroid instead of child stops #6047

habrahamsson-skanetrafiken · 2024-09-05T12:04:43Z

Summary

This PR adds the possibility to use a Station coordinate instead of the child stops when doing street routing. This affects both direct street routing and access/egress street routing.

The functionality can be configured on a per station basis during graph build.

The main thing this PR does is to separate the access/egress search into two steps:

Find all the directly accessible stops. From these stops you can directly board a transit. If using a station id this will be the child stops of that station, just like previously.
Run the access/egress street search. If using a station id with centroid routing this will use the centroid as origin for the search. If centroid routing is not enabled we will use the child stops as the origin, just like previously.

Example of new behaviour

Suppose you have a station "Station1" with a child stop "Stop1" and you search from the Station1 id to some destination. There are 3 basic ways an itinerary might start.

Original behaviour:

Direct:  Stop1 --[Walk leg]--> Destination
Access:  Stop1 --[Walk leg]--> OtherStop --[Bus leg]--> ...
Transit: Stop1 --[Bus  leg]--> ...

When routeToStationCentroid is enabled:

Direct:  Station1 --[Walk leg]--> Destination
Access:  Station1 --[Walk leg]--> OtherStop --[Bus leg]--> ...
Transit: Stop1    --[Bus  leg]--> ...

So the transit result is the same. The Direct and the Access cases now use the Station1 coordinate instead of the Stop1 coordinate.

See the issue for more details and motivation.

Also in this PR:

A little cleanup in the DirectTransferGenerator.
I changed a few methods to take the more explicit type AccessEgressType instead of boolean isEgress.
Changed TemporaryVerticesContainer to explicitly take the from and to locations instead of the whole RouteRequest that it wasn't using anyway.
Added wheelchair accessibility information to Stations.

Out of scope for this PR:

There are two new @Disabled tests in the StreetNearbyStopFinderTest that show an already existing off-by-1 error in the max stop counting. The problem is due to how the MaxCountSkipEdgeStrategy works. I think the stop counting should really use a SearchTerminationStrategy instead. I intend to fix this in a follow-up PR.

Issue

Closes #5848

Unit tests

Added tests both for the AccessEgressRouter and the StreetNearbyStopFinder.

Documentation

I documented to the best of my ability.

Bumping the serialization version id

Yes, this PR adds new vertices for the station centroids.

This commit should only change structure. No behaviour should be affected.

codecov · 2024-09-05T12:12:34Z

Codecov Report

Attention: Patch coverage is 74.78632% with 59 lines in your changes missing coverage. Please review.

Project coverage is 69.85%. Comparing base (5fee2ff) to head (a8674a9).
Report is 56 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...der/module/RouteToCentroidStationIdsValidator.java	0.00%	19 Missing ⚠️
...anner/graph_builder/module/StreetLinkerModule.java	27.27%	7 Missing and 1 partial ⚠️
...radapter/router/street/FlexAccessEgressRouter.java	28.57%	2 Missing and 3 partials ⚠️
..._builder/module/configure/GraphBuilderModules.java	0.00%	4 Missing ⚠️
...nner/street/search/TemporaryVerticesContainer.java	75.00%	2 Missing and 2 partials ⚠️
.../street/model/edge/TemporaryPartialStreetEdge.java	0.00%	3 Missing ⚠️
...ntripplanner/framework/geometry/WgsCoordinate.java	50.00%	2 Missing ⚠️
...builder/module/AddTransitModelEntitiesToGraph.java	66.66%	1 Missing and 1 partial ⚠️
...der/module/nearbystops/StreetNearbyStopFinder.java	87.50%	0 Missing and 2 partials ⚠️
...ntripplanner/netex/config/NetexFeedParameters.java	50.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6047      +/-   ##
=============================================
+ Coverage      69.84%   69.85%   +0.01%     
- Complexity     17464    17500      +36     
=============================================
  Files           1975     1976       +1     
  Lines          74614    74710      +96     
  Branches        7640     7645       +5     
=============================================
+ Hits           52117    52192      +75     
- Misses         19849    19868      +19     
- Partials        2648     2650       +2

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

src/main/java/org/opentripplanner/routing/api/request/request/JourneyRequest.java

leonardehrenfried · 2024-09-06T13:11:57Z

Would it be possible to see a screenshot of how the links now look like at Malmö Central?

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

src/main/java/org/opentripplanner/graph_builder/module/StreetLinkerModule.java

...n/java/org/opentripplanner/graph_builder/module/nearbystops/DirectlyConnectedStopFinder.java

...va/org/opentripplanner/routing/algorithm/raptoradapter/router/street/AccessEgressRouter.java

...rg/opentripplanner/routing/algorithm/raptoradapter/router/street/FlexAccessEgressRouter.java

leonardehrenfried · 2024-09-06T13:51:46Z

src/main/java/org/opentripplanner/routing/graph/index/StreetIndex.java

+  private static Map<FeedScopedId, TransitStationCentroidVertex> createTransitStationVertexMap(
+    Graph graph
+  ) {
+    var vertices = graph.getVerticesOfType(TransitStationCentroidVertex.class);


I need to think about this a bit. Iterating over all vertices can be pretty slow.

We are doing the same for the stop vertices. If it's an issue I can probably build both maps at the same time so we don't have to iterate twice

On my laptop this takes about 100ms for our dataset of 1.5M vertices. Since it's only done once during server startup, I don't think it's much to worry about?

The way to solve this is not to iterate over the station once and throw everything that need to done with them in the same loop, but to cashe the set of stop/station vertexes during graph build. I am not sure if we wan to keep these indexes around during routing, but if not the we need to split the data structure used for building and routing - probably a good idea anyway. I am not worried about the performance here, so this is probably something we can do later.

Germany has 23 million vertices so it would take about 1.5 seconds during start up. We have bigger fish to fry.

src/main/java/org/opentripplanner/standalone/config/buildconfig/NetexConfig.java

leonardehrenfried · 2024-09-06T13:55:25Z

src/main/java/org/opentripplanner/street/model/edge/StreetTransitStationCentroidLink.java

+  }
+
+  public String toString() {
+    return "StreetTransitStationCentroidLink(" + fromv + " -> " + tov + ")";


I know that many edges use this style of toString but I don't really like it. I would prefer ToStringBuilder. Lets discuss in the dev meeting.

I personally do not debug the AStar very often, but the notation here is a bit sharper than the ToStringBuilder would produce. Maybe the toString() can be implemented in the root of the hierarchy of Edge and Vertex. For example: getClass().getSimpleName() + "(" + fromv + " -> " + tov + ")";. In fact, the code above can be deleted, since the Edge#toString() does exactly as suggested (except using getName() not getSimpleName()). A more reusable approach might be needed. You probably need some way to override it, so a little more complex way to do it would make a reusable method - lik the Path#buildString() method.

Some think like this

public String toString() { return buildToString(b -> {}); } /** * Use this to construct a {@code toString()} in derived classes. The result will be: * {@code "%simple-class-name%(%fromv% -> %tov%" + body + ")"}. Note! There is no space * between {@code tov} and {@code body}. * * @param body Use this callback to add local content. */ protected String buildToString(Consumer<StringBuilder> body) { var buf = new StringBuilder(); buf.append(getClass().getSimpleName()) .append(" (") .append(fromv) .append(" -> ") .append(tov); if(body != null) { body.accept(buf); } return buf.append(')').toString(); }

I used your approach @t2gran and changed all edges to use the same toString format. This will change the output for some Edges that previously used the ToStringBuilder, but that shouldn't be a problem right?

leonardehrenfried · 2024-09-06T13:57:19Z

src/main/java/org/opentripplanner/street/model/edge/StreetTransitStationCentroidLink.java

+  }
+
+  protected int getStreetToStopTime() {
+    return 0;


Is this accurate? Wouldn't that give you the wrong time if you had to walk to a far away platform?

On the other hand it would probably hard to come up with a good formula for that and since it's a rarely used feature it's probably not very important.

As I understand it this is a feature for subways to simulate the time it takes to get from the street level to the underground stop? I think even if the stops are underground, you would want the station to be at street level. Like the main entrance or something like that. But I'm not completely sure

Very little documentation on the super method, so difficult to tell how this should be implemented.

But, if this is the time to travel from the OSM network to the Station centroid, then 0 is probably ok. We are going to use this to prefer stops closer to the centroid, over stops fare from it. We do not want the passenger to travel here -or provide any walk-legs/directions here. Adding a "line of sight" time might improve the results, but unsure how much.

Just to clarify: in general this PR won't prefer child stops closer to the centroid, since if you search from a station id you get direct accesses to all the child stops. Only if you actually have to walk to a stop that is not part of the station will you use the centroid as the starting point.

Regarding the streetToStopTime I think we agree that it's ok to leave it at 0 then.

src/main/java/org/opentripplanner/street/model/edge/StreetTransitStationCentroidLink.java

src/main/java/org/opentripplanner/street/model/vertex/TransitStationCentroidVertex.java

src/test/java/org/opentripplanner/netex/mapping/StopAndStationMapperTest.java

src/main/java/org/opentripplanner/street/model/vertex/Vertex.java

leonardehrenfried · 2024-09-06T14:09:50Z

src/main/java/org/opentripplanner/transit/model/site/Station.java

@@ -142,6 +155,10 @@ public ZoneId getTimezone() {
    return timezone;
  }

+  public Accessibility getWheelchairAccessibility() {


Can you add some javadoc? What does it mean for entire Station to be wheelchair-accessible?

I added a little bit of javadoc, but I'm unsure of how detailed I should be. I import the value directly from the netex StopPlace but I couldn't find any detailed specification of what this field means in netex. Since it's only used for this feature, right now the only implication is that the station centroid is reachable by wheelchair

The only way to interperet "wheelchair-accessible" on a station is that it is the default value for the stops, and then the stops should be able to override it. If so, "the only implication is that the station centroid is reachable by wheelchair" is wrong. Because, we should check ways and stops but not the free-edge to the centroid when doing wheelchair routing. Example: Default for Station is NOT_ASSESSABLE, then we would not be able to reach a ACCESSABLE stop in that station, just because we continue beyond the "passenger" destination to prioritise the path. Note! We reach the destination, before the search is terminated. If we can not traverse the last edge(to the centroid) we fail to find a valid result.

Ah, so I shouldn't check the wheelchair accessibility for the link to the centroid? In that case I don't really need to add the accessibility information to the Station.

And then maybe I should make the StreetTransitStationCentroidLink inherit from FreeEdge instead of from StreetTransitEntityLink?

As per the discussion in the developer meeting I removed the accessibility info from the Stations again.

src/test/java/org/opentripplanner/CoordinateHelper.java

t2gran · 2024-09-10T16:14:03Z

doc/user/BuildConfiguration.md

+List stations that should route to centroid.
+
+This field contains a list of station ids for which the centroid will be used instead
+of the stop coordinates.


Suggested change

of the stop coordinates.

of the stop coordinates when routing to the Station.

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/router/TransitRouter.java

t2gran · 2024-09-10T17:21:02Z

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/router/TransitRouter.java

      durationLimit,
      stopCountLimit
    );
+    var nearbyAccessEgress = AccessEgressMapper.mapNearbyStops(nearbyStops, type);


The nearbyAccessEgress name is a bit stange. All access are by definition nearby - they start at the point where you are, how long they are depend on the limit. So, removing the nearby part of the name here make it less confusing. The nearby is repeating the context, but that should be avoided.

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/router/TransitRouter.java

t2gran · 2024-09-10T17:29:23Z

...va/org/opentripplanner/routing/algorithm/raptoradapter/router/street/AccessEgressRouter.java

+   * Return a list of direct accesses/egresses that do not require any street search. This will
+   * return an empty list if the source/destination is not a stopId.
+   */
+  private static List<NearbyStop> findDirectAccessEgress(


Sorry, I do not understand this JavaDoc. direct in OTP means one-to-one search from origin to desstination, so using it together with access/egress is confusing. I know this was in the code before, but now it is in the public space - and need a proper name. Also, can you elaborate what "that do not require any street search" means?

Yeah, I was really struggling to come up with a good name for this concept and direct access/egress was the best that I could find, even though it is a bit confusing since we already have "direct" as a concept.

These are access/egresses to the stops that correspond to the location you search from/to. The stops you have implicit access to without having to use the street network. They have 0 length and don't show up as legs in the itinerary. If you search from a stop id that stop would have a direct access. If you search from a station the child stops would have direct accesses. And if you search from a coordinate you wouldn't have any direct access/egresses.

I designed it so that it's only within the AccessEgressRouter that we need to make a distinction between these and the street access/egresses.

Other names that I could come up with are:

free access/egress

immediate access/egress

instant access/egress

Or do you have any ideas for a better name?

findAccessEgressWithZeroDistance is my best suggestion

t2gran · 2024-09-26T10:52:55Z

src/main/java/org/opentripplanner/netex/NetexBundle.java

+    postValidation();
+


Please use the existing validation framework.

This code is missing DI, so you need to do it your self. The RouteToCentroidStopPlaceIdValidator should be added to the list of validators in the Validator, it should extend the AbstractHMapValidationRule and you will need to pass in the Set<String> routeToCentroidStopPlaceIds to the validator. This class is not using the routeToCentroidStopPlaceIds and should therefore not reference it either.

t2gran · 2024-09-26T11:08:16Z

.../org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/AccessEgressMapper.java

    if (!(nearbyStop.stop instanceof RegularStop)) {
      return null;
    }

    return new DefaultAccessEgress(
      nearbyStop.stop.getIndex(),
-      isEgress ? nearbyStop.state.reverse() : nearbyStop.state
+      accessOrEgress.isEgress() ? nearbyStop.state.reverse() : nearbyStop.state


uper minor thing, but when there is a natural order to things, it helps the reader to process it in that order:

// This does not follow the natural order accessOrEgress.isEgress() ? nearbyStop.state.reverse() : nearbyStop.state // This do, and is easier to read accessOrEgress.isAccess() ? nearbyStop.state : nearbyStop.state.reverse()

I will not request you to change this, but keep it in mind for the future - there are many of these in this PR, some was in the wrong order originally some are switched.

Yeah, since isEgress was used almost everywhere I got used to that order. But you're right that access is the more obvious default choice.

t2gran · 2024-09-26T11:11:11Z

src/main/java/org/opentripplanner/standalone/config/buildconfig/NetexConfig.java

+      .addRouteToCentroidStopPlaceIds(
+        config
+          .of("routeToCentroidStopPlaceIds")
+          .since(V2_6)


t2gran · 2024-09-26T11:30:07Z

src/main/java/org/opentripplanner/standalone/config/buildconfig/NetexConfig.java

@@ -158,6 +158,29 @@ private static NetexFeedParameters.Builder mapFilePatternParameters(
          .docDefaultValue(dft.ferryIdsNotAllowedForBicycle())
          .asStringSet(base.ferryIdsNotAllowedForBicycle())
      )
+      .addRouteToCentroidStopPlaceIds(


I do not see this as a netex feature - If so, there is a lot of changes needed in the main code. This is a generic feature witch can be applied to both NeTEx and GTFS.

t2gran · 2024-09-26T11:35:21Z

src/main/java/org/opentripplanner/standalone/config/buildconfig/NetexConfig.java

+            """
+            This field contains a list of stop place ids for which the centroid will be used instead
+            of the stop coordinates.
+
+            When doing street routing from/to a station the default behaviour is to route to any of
+            the stop place child quays. This can cause strange results for stop places that have quays
+            spread over a large area.
+
+            For some stop places you might instead wish to use the centroid of the stop place as the
+            source/destination for street routing. In this case the centroid will be  used both for
+            direct street search and for access/egress street search where the stop place is used as
+            the start/end of the access/egress.
+            """


Suggested change

"""

This field contains a list of stop place ids for which the centroid will be used instead

of the stop coordinates.

When doing street routing from/to a station the default behaviour is to route to any of

the stop place child quays. This can cause strange results for stop places that have quays

spread over a large area.

For some stop places you might instead wish to use the centroid of the stop place as the

source/destination for street routing. In this case the centroid will be used both for

direct street search and for access/egress street search where the stop place is used as

the start/end of the access/egress.

"""

"""

This field contains a list of station ids for which the centroid will be used instead

of the stops in routing. This apply to all routing to and from these stations including

direct street routing, flex and transit.

When doing street routing from/to a station the default behaviour is to route to any of

the stop place child quays. This can cause strange results for stop places that have quays

spread over a large area. For some stations you might instead wish to use the centroid of

the stop place as the origin/destination. Be aware that with this feature enabled, you will

get a waking leg from stops inside the station to the centroid.

"""

Actually, this is not really accurate. You won't get any walking legs for stops that belong to the station. Se my comment below with a screenshot to illustrate.

What do you think about this text instead?

This field contains a list of station ids for which street legs will start/end at the station centroid instead of the child stops. When searching from/to a station the default behaviour is to route from/to any of the stations child stops. This can cause strange results for stations that have stops spread over a large area. For some stations you might instead wish to use the centroid of the station as the origin/destination. In this case the centroid will be used both for direct street search and for access/egress street search where the station is used as the start/end of the access/egress. But transit that starts/ends at the station will work as usual without any additional street leg from/to the centroid.

t2gran · 2024-09-26T16:46:11Z

Please give this PR a better name - remember the name should give a ProductOvner a clue to what this feature is - it is listed in the changelog. Maybe: "Enable routing to Station centroid instead of child stops"

t2gran · 2024-09-26T17:31:36Z

Here is my suggestion for doing the doing the validation in a GraphBuilder module. You should be able to cherry-pick the commit from the pullrequest-6047 branch in OTP main. I did not fix all the tests, just the main code.

Moving the Station "tagging" is a bit more complicated. So, you can leave it for now. We can either add the same code to the GTFS import, or do the necessary refactoring to inject this in the TransModel builder. Right now the mapping code is doing the Stop <-> Station linking, witch means that changing a Station/Stop is difficult. There should be an aggregate here so this was possible.

habrahamsson-skanetrafiken · 2024-09-30T13:42:26Z

Here is an image to illustrate the change in behavior for three cases. All three use Malmö C as their start point.

Transit directly from a child stop.
Access when you walk to a nearby stop and take transit from there.
Direct street search with no transit.

The white legs are walk legs.

Note that in the 1st case there is no change. We don't add walk legs for transit from stops that belong to Malmö C. The only thing that changes is that the street legs start at the centroid for cases 2 and 3.

src/main/java/org/opentripplanner/graph_builder/module/StreetLinkerModule.java

t2gran · 2024-10-03T15:11:06Z

src/main/java/org/opentripplanner/graph_builder/module/StreetLinkerModule.java

+          station,
+          new TraverseModeSet(TraverseMode.WALK),
+          LinkingDirection.BOTH_WAYS,
+          createLinkEdges


Suggested change

createLinkEdges

linkStationAndStreetVetex

I sometimes add linkStationAndStreetVetexOp to make sure it is clear that it is a lambda, do you know any best practices here.

I also sometimes like to use a suffix for clarification. But I think in most cases having the name be a verb is clear enough.

We should use stationAndStreetVetexLinker

t2gran · 2024-10-03T15:21:20Z

...va/org/opentripplanner/routing/algorithm/raptoradapter/router/street/AccessEgressRouter.java

+   * Return a list of direct accesses/egresses that do not require any street search. This will
+   * return an empty list if the source/destination is not a stopId.
+   */
+  private static List<NearbyStop> findDirectAccessEgress(


findAccessEgressWithZeroDistance is my best suggestion

src/main/java/org/opentripplanner/graph_builder/module/StreetLinkerModule.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>

habrahamsson-skanetrafiken added 2 commits August 29, 2024 16:05

Refactor: Simplify DirectTransferGenerator

25068f1

This commit should only change structure. No behaviour should be affected.

Add accessibility to station

4b65b74

habrahamsson-skanetrafiken added New Feature Skanetrafiken On skanetrafikens roadmap bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Sep 5, 2024

habrahamsson-skanetrafiken requested a review from a team as a code owner September 5, 2024 12:04

Implementation of route to centroid coordinate

2047421

habrahamsson-skanetrafiken force-pushed the access-egress-station-routing branch from 3c5c7b4 to 2047421 Compare September 5, 2024 12:06

t2gran reviewed Sep 5, 2024

View reviewed changes

src/main/java/org/opentripplanner/routing/api/request/request/JourneyRequest.java Outdated Show resolved Hide resolved

habrahamsson-skanetrafiken requested a review from leonardehrenfried September 5, 2024 14:22

Revert non-allowed dependency

41a9870

habrahamsson-skanetrafiken force-pushed the access-egress-station-routing branch from 591201c to 41a9870 Compare September 6, 2024 08:21

habrahamsson-skanetrafiken requested a review from t2gran September 6, 2024 09:31