Add 'transferParametersForMode' build config field #6215

VillePihlava · 2024-11-01T11:15:40Z

Summary

This PR adds the transferParametersForMode build config field for configuring transfers. These parameters can be configured for all transfer modes:

disableDefaultTransfers: allows disabling default transfer calculations
maxTransferDuration: allows configuring a mode-specific maxTransferDuration
carsAllowedStopMaxTransferDuration: allows changing max transfer durations between stops that allow cars

This is a follow-up PR related to #5966. Example configuration:

  "transferParametersForMode": {
    "CAR": {
      "disableDefaultTransfers": true,
      "carsAllowedStopMaxTransferDuration": "3h"
    },
    "BIKE": {
      "maxTransferDuration": "30m",
      "carsAllowedStopMaxTransferDuration": "3h"
    }
  },

Issue

This is a follow-up PR related to issue #5875.

Unit tests

I created new unit tests for testing the changes to the DirectTransferGenerator

Documentation

Changes to BuildConfiguration.md

Bumping the serialization version id

This change affects how transfers are calculated and adds a new field to the build-config.json file.

…th the 'carsAllowedStopMaxTransferDurationsForMode' build config parameter.

…ig parameter.

codecov · 2024-11-01T11:21:38Z

Codecov Report

Attention: Patch coverage is 91.76471% with 14 lines in your changes missing coverage. Please review.

Project coverage is 69.76%. Comparing base (448ad8f) to head (186d20c).
Report is 1 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
.../graph_builder/module/DirectTransferGenerator.java	89.83%	6 Missing and 6 partials ⚠️
.../standalone/config/buildconfig/TransferConfig.java	85.71%	1 Missing ⚠️
...e/config/buildconfig/TransferParametersMapper.java	95.23%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6215      +/-   ##
=============================================
+ Coverage      69.72%   69.76%   +0.04%     
- Complexity     18016    18045      +29     
=============================================
  Files           2057     2060       +3     
  Lines          76967    77097     +130     
  Branches        7844     7854      +10     
=============================================
+ Hits           53666    53788     +122     
- Misses         20550    20556       +6     
- Partials        2751     2753       +2

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

habrahamsson-skanetrafiken · 2024-11-05T10:47:23Z

There are some conflicts

…into car-transferrequest-filtering

optionsome

I've had too many reviews today already so I didn't go properly through this. However, inspired by some concerns from @t2gran, I think to avoid issues we should do a couple of things:

Introduce carsAllowedStopMaxAccessEgressDurationsForMode. This is to prevent weird back-and-forth transfers to avoid access and egress duration limitations.
Prevent these transfers from being used by other street modes. I don't know if this is already done or not but I think at least previously all the transfers were in the same collection. This could again lead to weird back and forth travel by walking for example to escape some other restrictions.

optionsome · 2024-11-08T12:52:25Z

application/src/main/java/org/opentripplanner/api/parameter/QualifiedModeSet.java

-            mBuilder.withEgressMode(StreetMode.WALK);
-            mBuilder.withDirectMode(StreetMode.CAR);
+            // This is used in transfer cache request calculations.
+            mBuilder.withAllStreetModes(StreetMode.CAR);


I know this builder method wasn't introduced in this pr but this name is really confusing. Idk what we call Access+Transfer+Egress+Direct. It's difficult to come up with a name that doesn't confuse you into thinking that all modes are enabled. Just making this singular would maybe make it less confusing.

I changed it to set each mode separately for more clarity

optionsome · 2024-11-08T13:09:21Z

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

@@ -60,6 +68,23 @@ public DirectTransferGenerator(
    this.issueStore = issueStore;
    this.radiusByDuration = radiusByDuration;
    this.transferRequests = transferRequests;
+    this.carsAllowedStopMaxTransferDurationsForMode = DurationForEnum.of(StreetMode.class).build();


Maybe we should always include this with some default value that is the same as the normal transfer duration limit.

This has changed quite a lot, but you can look at how I handle an empty parameter

optionsome · 2024-11-08T13:09:56Z

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

+      .stream()
+      .map(StopLocation::getId)
+      .map(graph::getStopVertexForStopId)
+      .filter(TransitStopVertex.class::isInstance) // filter out null values if no TransitStopVertex is found for ID


Inline comments are not allowed.

VillePihlava · 2024-11-12T11:14:30Z

I'm changing this to a draft for now. I'll return to this once the work on the separate transfer request collections for modes has been completed.

… tests.

…nsferrequest-filtering

…into car-transferrequest-filtering

optionsome · 2025-01-10T19:28:42Z

Should this bump the serialization id since the path transfers are now mode dependent?

optionsome · 2025-01-10T19:44:19Z

doc/user/BuildConfiguration.md

+| [maxDataImportIssuesPerFile](#maxDataImportIssuesPerFile)                |       `integer`      | When to split the import report.                                                                                                                               | *Optional* | `1000`                            |  2.0  |
+| maxElevationPropagationMeters                                            |       `integer`      | The maximum distance to propagate elevation to vertices which have no elevation.                                                                               | *Optional* | `2000`                            |  1.5  |
+| [maxStopToShapeSnapDistance](#maxStopToShapeSnapDistance)                |       `double`       | Maximum distance between route shapes and their stops.                                                                                                         | *Optional* | `150.0`                           |  2.1  |
+| maxTransferDuration                                                      |      `duration`      | Transfers up to this duration with the default walk speed value will be pre-calculated and included in the Graph.                                              | *Optional* | `"PT30M"`                         |  2.1  |


Should this parameter be moved to be inside transferParameters? Also, I think the description is wrong since this can be used for other types of transfers as well. There might be one or two other parameters that could be moved to be under transferParameters such as discardMinTransferTimes and maybe stationTransferPreference.

As per the OTP meeting discussion, I renamed transferParameters to transferParametersForMode. I also changed the summary for maxTransferDuration to Transfers up to this duration with a mode-specific speed value will be pre-calculated and included in the Graph.

optionsome · 2025-01-10T19:57:18Z

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

+  Duration carsAllowedStopMaxTransferDuration,
+  boolean disableDefaultTransfers
+) {
+  public static final Duration DEFAULT_MAX_TRANSFER_DURATION = Duration.ZERO;


Isn't the default 30 minutes? I think the default should be set only in one place and then used from there elsewhere.

I set both of the Duration.ZERO to null

optionsome · 2025-01-10T19:59:00Z

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

+  boolean disableDefaultTransfers
+) {
+  public static final Duration DEFAULT_MAX_TRANSFER_DURATION = Duration.ZERO;
+  public static final Duration DEFAULT_CARS_ALLOWED_STOP_MAX_TRANSFER_DURATION = Duration.ZERO;


I don't know if the default value for this should be the same as the normal default or null.

I set both of the Duration.ZERO to null

optionsome · 2025-01-10T20:04:40Z

application/src/main/java/org/opentripplanner/standalone/config/BuildConfig.java

@@ -287,6 +293,7 @@ When set to true (it is false by default), the elevation module will include the
          "Transfers up to this duration with the default walk speed value will be pre-calculated and included in the Graph."
        )
        .asDuration(Duration.ofMinutes(30));
+    transferParametersForMode = TransferConfig.map(root, "transferParameters");


This variable should be renamed if we decide to make the transferParameters used for other transfer parameters as well.

As per the OTP meeting discussion, I renamed transferParameters to transferParametersForMode

optionsome · 2025-01-10T20:13:01Z

...rc/main/java/org/opentripplanner/standalone/config/buildconfig/TransferParametersMapper.java

+import org.opentripplanner.graph_builder.module.TransferParameters;
+import org.opentripplanner.standalone.config.framework.json.NodeAdapter;
+
+public class TransferParametersMapper {


These parameters don't show up in the BuildConfiguration.md now. If I remember correctly, you should at least add them to https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/resources/standalone/config/build-config.json and https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/java/org/opentripplanner/generate/doc/framework/NodeAdapterHelper.java#L8-L20 as well.

I added changes to both files

optionsome · 2025-01-10T20:15:02Z

...rc/main/java/org/opentripplanner/standalone/config/buildconfig/TransferParametersMapper.java

+        .description(
+          """
+This parameter configures additional transfers to be calculated for the specified mode only between stops that have trips with cars.
+The transfers are calculated for the mode in a range based on the given duration.
+By default, these transfers are not calculated for the specified mode.
+"""
+        )


Since this parameter is rather odd on the first glance, you could explain a bit here that this might be useful when there are ferries that sort of extend the road network.

Yeah, it's a difficult concepts so needs a bit more docs.

I added more documentation

optionsome · 2025-01-10T20:16:46Z

...rc/main/java/org/opentripplanner/standalone/config/buildconfig/TransferParametersMapper.java

+    builder.withDisableDefaultTransfers(
+      c
+        .of("disableDefaultTransfers")
+        .summary("This disables default transfer calculations.")


You could add a bit more descriptive description for what the default transfer calculations means and why you might want to do this.

I added more documentation

leonardehrenfried

If you fix @optionsome 's points I can approve this.

VillePihlava · 2025-01-14T10:47:27Z

Should this bump the serialization id since the path transfers are now mode dependent?

The mode-dependency for PathTransfers was added in another PR. Because this PR changes how transfer calculations can be configured, I think the serialization id should be bumped.

optionsome · 2025-01-14T15:21:39Z

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

@@ -117,47 +142,32 @@ public void buildGraph() {
        LOG.debug("Linking stop '{}' {}", stop, ts0);

        // Calculate default transfers.
-        for (RouteRequest transferProfile : transferRequests) {
+        for (RouteRequest transferProfile : transferConfiguration.defaultTransferRequests) {


I think it's better if you use the record fields through the getters instead of accessing the fields directly.

This has now been changed

optionsome · 2025-01-14T15:23:26Z

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

+    List<RouteRequest> carsAllowedStopTransferRequests = new ArrayList<>();
+    List<RouteRequest> flexTransferRequests = new ArrayList<>();
+    HashMap<StreetMode, NearbyStopFinder> defaultNearbyStopFinderForMode = new HashMap<>();
+    /* These are used for calculating transfers only between carsAllowedStops. */


Use normal comments inside methods.

These were from a previous commit. I changed them in this file everywhere I could find them

optionsome · 2025-01-14T15:42:09Z

doc/user/BuildConfiguration.md

+```
+
+
+<h3 id="tpfm_BIKE_carsAllowedStopMaxTransferDuration">carsAllowedStopMaxTransferDuration</h3>


Now the same fields are documented for all modes (specified in the example build-config.json) which is not what we want since I doubt we will have mode specific parameters. I don't know what is the best way to get rid of this. One option would be to have a structure like:

{ "transferParametersForModes": [ {"mode": "CAR", "maxTransferDuration": "30m"} ] }

instead of having this kind of an enum map.

As discussed this has been left as is. I added a topic discussion in the dev meeting thread

leonardehrenfried · 2025-01-15T08:51:31Z

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

+            createPathTransfer(sd.stop, stop, sd, distinctTransfers, mode);
+          }
+        }
+        // Calculate transfers between stops that are visited by trips that allow cars, if configured.


This method is getting too long. Please split it up into the various phases (default, flex, cars allowed) and then convert these line comments into javadoc.

Afterwards I'm satisfied and will approve.

I split this into three functions

…into car-transferrequest-filtering

optionsome · 2025-01-21T08:56:32Z

I think the issue with the documentation getting generated in a wrong way still exists. I think we should investigate fixing it in the scope of this pr.

VillePihlava added 3 commits November 1, 2024 11:58

Add ability to create special transfer requests for cars and bikes wi…

36336bf

…th the 'carsAllowedStopMaxTransferDurationsForMode' build config parameter.

Add tests for 'carsAllowedStopMaxTransferDurationsForMode' build conf…

f1380e3

…ig parameter.

Move if statement outside of for loop.

2175e64

VillePihlava requested a review from a team as a code owner November 1, 2024 11:15

habrahamsson-skanetrafiken requested review from optionsome and leonardehrenfried November 5, 2024 10:47

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

6418e26

…into car-transferrequest-filtering

optionsome reviewed Nov 8, 2024

View reviewed changes

VillePihlava marked this pull request as draft November 12, 2024 11:14

VillePihlava added 3 commits December 3, 2024 08:43

Fixes based on review comments.

e1e9bbb

Add TransferParameters to build config.

89e617f

Remove mentions of carsAllowedStopMaxTransferDurationsForMode and fix…

a1e0807

… tests.

VillePihlava changed the title ~~Add carsAllowedStopMaxTransferDurationsForMode build config parameter~~ Add 'transfers build config field Dec 3, 2024

VillePihlava changed the title ~~Add 'transfers build config field~~ Add 'transfers' build config field Dec 3, 2024

VillePihlava added 6 commits December 5, 2024 10:00

Rename variables and small improvements.

21bb307

Change test.

573078a

Add documentation for build config fields.

3fd5bf5

Add logging for transfers.

35c98bc

Merge branch 'split-transfers-by-mode-pathtransfer-mode' into car-tra…

3a34aa4

…nsferrequest-filtering

Fix merge issues.

97238a3

VillePihlava mentioned this pull request Dec 9, 2024

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

Merged

VillePihlava added 6 commits December 10, 2024 12:02

Add tests and small improvements.

41a0674

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

1a3f846

…into car-transferrequest-filtering

Refactor transfer parameter parsing.

2e048f9

Remove duplicate test.

c90c3ec

Simplify implementation by removing changes to StreetNearbyStopFinder.

30cc925

Remove unnecessary parameter from function call.

2e57c2f

Add comments and refactor transfer generation.

4d599a2

VillePihlava changed the title ~~Add 'transfers' build config field~~ Add 'transferParameters' build config field Jan 7, 2025

Rename transfers to transferParameters in documentation.

2eb0912

optionsome requested review from optionsome and leonardehrenfried January 9, 2025 13:41

optionsome requested changes Jan 10, 2025

View reviewed changes

leonardehrenfried reviewed Jan 13, 2025

View reviewed changes

VillePihlava added 5 commits January 14, 2025 10:38

Change default values from Duration.ZERO to null.

57e03b4

Add documentation.

7de4a39

Rename transferParameters to transferParametersForMode.

8e87974

Change documentation for maxTransferDuration.

d0fbae9

Add documentation to standalone build-config.json and NodeAdapterHelper.

f385061

VillePihlava changed the title ~~Add 'transferParameters' build config field~~ Add 'transferParametersForMode' build config field Jan 14, 2025

optionsome added bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Config Change Improvement labels Jan 14, 2025

optionsome requested review from leonardehrenfried and optionsome January 14, 2025 15:18

optionsome requested changes Jan 14, 2025

View reviewed changes

leonardehrenfried reviewed Jan 15, 2025

View reviewed changes

VillePihlava added 4 commits January 16, 2025 09:54

Use method to get field instead of the field directly.

afa1b16

Change comment style.

c649ab7

Split transfer generation into methods.

91782ef

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

186d20c

…into car-transferrequest-filtering

VillePihlava requested review from leonardehrenfried and optionsome January 21, 2025 08:54

leonardehrenfried approved these changes Jan 21, 2025

View reviewed changes

		```


		<h3 id="tpfm_BIKE_carsAllowedStopMaxTransferDuration">carsAllowedStopMaxTransferDuration</h3>

Add 'transferParametersForMode' build config field #6215

Are you sure you want to change the base?

Add 'transferParametersForMode' build config field #6215

Conversation

VillePihlava commented Nov 1, 2024 • edited Loading

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

habrahamsson-skanetrafiken commented Nov 5, 2024

optionsome left a 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 Nov 12, 2024

optionsome commented Jan 10, 2025

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

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 left a comment

Choose a reason for hiding this comment

VillePihlava commented Jan 14, 2025

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

optionsome commented Jan 21, 2025

VillePihlava commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading