Fix bug in heuristics cost calculation for egress legs

[habrahamsson-skanetrafiken]

Summary

This PR fixes an issue in the heuristics optimization in raptor where the stopTransferCost is not correctly subtracted for egress legs. We found a case in skånetrafiken where this caused an optimal path to be discarded.

The issue is that during cost calculation, the stopTransferCost is added to all alightings, and if it later turns out that this is an egress leg that cost is later subtracted. However this is not done in the heuristic cost calculation. Thus there are some cases where the heuristics will give a higher cost estimate than what the actual cost is going to be and this causes incorrectly pruned paths.

I updated the costCalculator to subtract the stop transfer cost and also renamed calculateMinCost() to calculateRemainingMinCost() in order to clarify that it doesn't do a generic cost calculation.

Unit tests

• Added a test case for the defaultCostCalculator.
• Added a regression test in raptor moduletests. (Is this the correct place for this?)

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.81%. Comparing base (c1a9196) to head (5b6cfda).
Report is 94 commits behind head on dev-2.x.

❗ Current head 5b6cfda differs from pull request most recent head a5d91fb. Consider uploading reports for the commit a5d91fb to get more accurate results

Files	Patch %	Lines
...toradapter/transit/cost/PatternCostCalculator.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5783      +/-   ##
=============================================
- Coverage      67.81%   67.81%   -0.01%     
+ Complexity     16532    16530       -2     
=============================================
  Files           1906     1906              
  Lines          72275    72282       +7     
  Branches        7443     7444       +1     
=============================================
+ Hits           49015    49017       +2     
- Misses         20740    20743       +3     
- Partials        2520     2522       +2     

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

[habrahamsson-skanetrafiken]

I ran the speed tests on this change and it doesn't look like it affects performance.

[t2gran]

boardAndAlightCost is not the same as transferCosts - we can discuss this in the OTP dev meeting

[habrahamsson-skanetrafiken]

My reasoning was that it looks like the costCalculator treats the boardAlightCosts as transferCosts, and thus it would be a little bit clearer how this parameter will be used if this was reflected in the signature for the createCostCalculator() method.

But as discussed in the meeting it might be better to draw the line at the module border. I'm fine with reverting this.

[t2gran]

Sorry for the confution here. This is the case:

• Raptor does not have transferCost only alight and board cost, so the transfer cost is added either to boarding(if symetrical) or in this case split in two and added to both alight and boarding - to stops may have different cost - hence we need to add a cost for both alighting and boarding.

This "mess" should probably be visited again and cleaned up - the cost model have grown and it might be just a bit more clear if Raptor supported board, alight and transfer costs - unsure what the performance overhead it. Performance was the original reason for why this is like it is. Compressing stop information in memory increases the performance.

So this feature comes from NeTEX where the name is transfer, but since we are adding the cost to alight and boarding (both ends) I think the name in the OTP model i wrong - the mapping should happen when going from stopPriority to cost. A few comments in the model would have helped as well.

In TransitRoutingConfig and TransitTuningParameters this should be renamed to something like boardAndAlightCostForStopTransferPriority - a shorter name and a better java doc is probably the best solution.

[t2gran]

Sumarized: boardAndAlight cost is added twice (both ends of a transfer), while transferCost is added once.

[habrahamsson-skanetrafiken]

Thanks for that explanation!

The confusing thing about the name "stopBoardAlightCosts" is that it sounds like it should be applied for the first boarding and the last alighting as well. There is nothing that indicates that it is specific to transfers. But calling it stopBoardAlightTransferCosts may be a bit long?

Here are some possible alternatives for the name of this concept:

• stopTransferCosts
• stopBoardAlightCosts
• stopBoardAlightTransferCosts
• stopBoardAlightDuringTransferCosts
• stopHalfTransferCosts

Do you think we should go with "stopBoardAlightCosts"?

I'm ok with that and think the naming is perhaps less important than being consistent, so I'm happy to make the naming consistent and will add some javadoc.

[t2gran]

I think we can use this stopBoardAlightTransferCosts with JavaDoc.

Doc in one place and link to doc in other places.

[t2gran]

Thank you for this fix. It looks good. If you want to revert the name change or do the suggested refactor/javadoc is a bit up to you - I would appreciate if you added a bit of JavaDoc (blame on me and Hannes).

[t2gran]

Sorry for the confution here. This is the case:

• Raptor does not have transferCost only alight and board cost, so the transfer cost is added either to boarding(if symetrical) or in this case split in two and added to both alight and boarding - to stops may have different cost - hence we need to add a cost for both alighting and boarding.

This "mess" should probably be visited again and cleaned up - the cost model have grown and it might be just a bit more clear if Raptor supported board, alight and transfer costs - unsure what the performance overhead it. Performance was the original reason for why this is like it is. Compressing stop information in memory increases the performance.

So this feature comes from NeTEX where the name is transfer, but since we are adding the cost to alight and boarding (both ends) I think the name in the OTP model i wrong - the mapping should happen when going from stopPriority to cost. A few comments in the model would have helped as well.

In TransitRoutingConfig and TransitTuningParameters this should be renamed to something like boardAndAlightCostForStopTransferPriority - a shorter name and a better java doc is probably the best solution.

[t2gran]

By the way, understanding the diagram here is key to understand this cost calculation. The cost is calculated for each stop arrival - everyting in between can be added in random order - witch is used to optimize, but with increased complexity and the bug this PR fixes as a result.

[habrahamsson-skanetrafiken]

After discussions I'll merge this fix and do the renaming of the stopBoardAlightCosts / stopTransferCosts in a separate PR