Add 'transferParametersForMode' build config field by VillePihlava · Pull Request #6215 · opentripplanner/OpenTripPlanner

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 50 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

-            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

    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

+      .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:44:19Z

+| [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

+  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

+  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

          "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

+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

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

+    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


        // 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

+    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

+```
+
+
+<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

+            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 leonardehrenfried and optionsome 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

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 This PR might require the configuration to be updated. !Improvement A functional improvement or micro feature 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

optionsome approved these changes Jan 23, 2025

View reviewed changes

optionsome merged commit 0c2ed2d into opentripplanner:dev-2.x Jan 23, 2025

optionsome deleted the car-transferrequest-filtering branch January 23, 2025 13:00

t2gran pushed a commit that referenced this pull request Jan 23, 2025

Add changelog entry for #6215 [ci skip]

b74638f

t2gran pushed a commit that referenced this pull request Jan 23, 2025

Bump serialization version id for #6215

e9a8742

t2gran added this to the 2.7 (next release) milestone Jan 29, 2025

		```


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

Uh oh!

Conversation

VillePihlava commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

Uh oh!

codecov Bot commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

habrahamsson-skanetrafiken commented Nov 5, 2024

Uh oh!

optionsome 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!

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!

VillePihlava commented Nov 12, 2024

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!

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!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

VillePihlava commented Jan 14, 2025

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!

VillePihlava commented Nov 1, 2024 •

edited

Loading

codecov Bot commented Nov 1, 2024 •

edited

Loading