Skip to content

Conversation

@miklcct
Copy link
Contributor

@miklcct miklcct commented Aug 6, 2025

Summary

This adds a feature flag to limit the generation of bike transfers to stops serving bikes or empty stops only.

Issue

Improves #6781

Unit tests

Added

Documentation

New build configuration option added

Changelog

Bumping the serialization version id

Not required

@miklcct miklcct force-pushed the optimise-bike-transfer branch from 2adc320 to 0870b27 Compare August 7, 2025 13:54
@miklcct miklcct force-pushed the optimise-bike-transfer branch from 0870b27 to 4c0f483 Compare August 7, 2025 13:58
@codecov
Copy link

codecov bot commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 94.73684% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.21%. Comparing base (7d31996) to head (d23ef64).
⚠️ Report is 146 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
.../graph_builder/module/DirectTransferGenerator.java 92.85% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6787      +/-   ##
=============================================
+ Coverage      72.15%   72.21%   +0.05%     
- Complexity     19782    19852      +70     
=============================================
  Files           2151     2155       +4     
  Lines          79981    80084     +103     
  Branches        8061     8084      +23     
=============================================
+ Hits           57713    57831     +118     
+ Misses         19422    19407      -15     
  Partials        2846     2846              

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

@miklcct
Copy link
Contributor Author

miklcct commented Aug 7, 2025

On a test with London data, this feature flag has resulted in the graph.obj file decreased from 10.0 GB to 2.3 GB with 3 walk transfers and 1 bike transfer, and the number of BIKE transfers has decreased from 9938935 to 1459294.

It isn't relevant for now but if we decide to support bike rental or bike parking for transfers, this optimization shouldn't apply
@miklcct miklcct marked this pull request as ready for review August 8, 2025 13:45
@miklcct miklcct requested a review from a team as a code owner August 8, 2025 13:45
@t2gran t2gran added this to the 2.8 (next release) milestone Aug 11, 2025
Copy link
Contributor

@VillePihlava VillePihlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the functionality. Can you take a look at how the carsAllowedStopMaxTransferDuration is implemented in https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/main/java/org/opentripplanner/standalone/config/buildconfig/TransferParametersMapper.java and change your implementation to use the build config

@miklcct miklcct force-pushed the optimise-bike-transfer branch from ff5f1af to 576add2 Compare August 12, 2025 13:24
Copy link
Contributor

@VillePihlava VillePihlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build config changes look good. I don't fully understand the realtime aspect of the PR. What kind of realtime updates exist at graph build time that are not in the normal timetables? @miklcct Could you write a short explanation of you use case/goal. It could be that you are trying to do something that is currently not possible.

Copy link
Contributor

@VillePihlava VillePihlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple comments related to documentation and functionality. I'll take a look at the tests at a later date.

@VillePihlava VillePihlava changed the title limit bike transfer to stops serving bikes only Add bikesAllowedStopMaxTransferDuration to build config to limit bike transfers Aug 18, 2025
Copy link
Contributor

@VillePihlava VillePihlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the previous feedback, I found a couple of things related to the tests. When all the changes have been done, I think this PR should be ready

@optionsome optionsome added the +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR label Aug 19, 2025
@VillePihlava
Copy link
Contributor

You should also add the field to this build config (similar to the line I linked) so that it automatically generates documentation:

@t2gran t2gran modified the milestones: 2.8, 2.9 (next release) Sep 10, 2025
@miklcct miklcct requested a review from optionsome September 11, 2025 17:01
Copy link
Contributor

@VillePihlava VillePihlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I found a few things

VillePihlava
VillePihlava previously approved these changes Sep 26, 2025
@optionsome optionsome removed their request for review September 29, 2025 13:02
@t2gran t2gran assigned t2gran and unassigned t2gran Oct 1, 2025
@optionsome optionsome requested a review from t2gran October 7, 2025 09:09
t2gran
t2gran previously approved these changes Oct 8, 2025
# Conflicts:
#	application/src/test/java/org/opentripplanner/routing/algorithm/GraphRoutingTest.java
VillePihlava
VillePihlava previously approved these changes Oct 10, 2025
@VillePihlava VillePihlava requested a review from t2gran October 10, 2025 13:06
# Conflicts:
#	doc/user/BuildConfiguration.md
@t2gran t2gran self-assigned this Oct 22, 2025
@miklcct miklcct merged commit dd12520 into opentripplanner:dev-2.x Oct 23, 2025
8 checks passed
t2gran pushed a commit that referenced this pull request Oct 23, 2025
t2gran pushed a commit that referenced this pull request Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants