Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer cost limit #5516

Merged

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Nov 16, 2023

Summary

As discussed in today's dev meeting and described in #5509 absurdly high transfer costs can lead to an integer overflow in RAPTOR.

This PR adds a safety limit when transfer costs are being calculated. This limit is still pretty high (several days of transit-equiavalence) but well below Integer.MAX_VALUE.

Issue

Closes #5509

Unit tests

✔️

Documentation

Javadoc.

cc @miles-grant-ibigroup @binh-dam-ibigroup

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (f20ffe8) 66.90% compared to head (428970b) 66.97%.
Report is 74 commits behind head on dev-2.x.

Files Patch % Lines
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java 17.64% 14 Missing ⚠️
...ext/stopconsolidation/StopConsolidationModule.java 79.31% 6 Missing ⚠️
...tion/internal/DefaultStopConsolidationService.java 83.87% 3 Missing and 2 partials ⚠️
...planner/graph_builder/GraphBuilderDataSources.java 0.00% 5 Missing ⚠️
...tion/configure/StopConsolidationServiceModule.java 0.00% 4 Missing ⚠️
..._builder/module/configure/GraphBuilderModules.java 0.00% 4 Missing ⚠️
...m/filterchain/ItineraryListFilterChainBuilder.java 55.55% 3 Missing and 1 partial ⚠️
...rithm/mapping/RouteRequestToFilterChainMapper.java 33.33% 2 Missing and 2 partials ⚠️
...ext/stopconsolidation/StopConsolidationParser.java 91.42% 3 Missing ⚠️
...rg/opentripplanner/graph_builder/GraphBuilder.java 0.00% 3 Missing ⚠️
... and 6 more
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5516      +/-   ##
=============================================
+ Coverage      66.90%   66.97%   +0.06%     
- Complexity     15734    15821      +87     
=============================================
  Files           1823     1833      +10     
  Lines          70366    70599     +233     
  Branches        7403     7410       +7     
=============================================
+ Hits           47080    47281     +201     
- Misses         20821    20845      +24     
- Partials        2465     2473       +8     

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

@leonardehrenfried leonardehrenfried force-pushed the transfer-cost-limit branch 2 times, most recently from ba043a1 to 099d65e Compare November 16, 2023 21:29
@optionsome
Copy link
Member

Is this problem unique to transfers? Could this happens in street routing elsewhere? I'm just wondering should we slightly lower the cost of wheelchair traverse on these non-wheelchair friendly edges?

@leonardehrenfried leonardehrenfried force-pushed the transfer-cost-limit branch 2 times, most recently from d63c58c to ba0c3f9 Compare November 16, 2023 22:32
@leonardehrenfried leonardehrenfried force-pushed the transfer-cost-limit branch 2 times, most recently from 025a299 to 2a6bc4d Compare November 20, 2023 14:06
@leonardehrenfried
Copy link
Member Author

Is this problem unique to transfers? Could this happens in street routing elsewhere? I'm just wondering should we slightly lower the cost of wheelchair traverse on these non-wheelchair friendly edges?

It's technically possible for access/egress searches but very, very unlikely since there a proper A* calculates the cost and prunes away bad choices. In a transfer you don't have an A* and simply re-traverse the pre-computed list of edges with new street preferences.

As @slvlirnoff has pointed out, the cost calculation for wheelchairs increases probably a little too explosively because we multiply the reluctances with each other instead of adding them.

So we currently do

cost = edgeLength * walkReluctance * slopeReluctance * stairsReluctance * ...

when we probably want something like this

cost = edgeLength * (walkReluctance + slopeReluctance + stairsReluctance + ...)

Refactoring this quite complicated because the StreetEdge has a lot of spaghetti code. Maybe one day I will get around to doing that.

@leonardehrenfried leonardehrenfried added the IBI Developed by or important for IBI Group label Nov 21, 2023
@t2gran
Copy link
Member

t2gran commented Nov 21, 2023

On cost = edgeLength */+ walkReluctance */+ slopeReluctance */+ stairsReluctance */+ ...

I would like to discuss this with @abyrd. I think using + and not * may make sense, but it changes the cost calculations a lot an it will require us to tune OTP again. My first reaction is that it makes the reluctance independent of each other - witch I think is a good thing.

Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

This looks good, only a few comments to the test. I am ready to approve this after the small fixes is applied.

@leonardehrenfried leonardehrenfried merged commit 7e7b689 into opentripplanner:dev-2.x Nov 22, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Nov 22, 2023
@leonardehrenfried leonardehrenfried deleted the transfer-cost-limit branch November 22, 2023 11:47
@t2gran t2gran added this to the 2.5 (next release) milestone Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IBI Developed by or important for IBI Group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negative cost of itinerary causes exception during itinerary filtering
4 participants