Skip to content

Support for second "cost" criteria in McRaptor - #4996

Merged
t2gran merged 26 commits into
opentripplanner:dev-2.xfrom
entur:otp2_c2_part_1
Mar 31, 2023
Merged

Support for second "cost" criteria in McRaptor #4996
t2gran merged 26 commits into
opentripplanner:dev-2.xfrom
entur:otp2_c2_part_1

Conversation

@t2gran

@t2gran t2gran commented Mar 23, 2023

Copy link
Copy Markdown
Member

Summary

This PR focus on adding a second criteria(c2) to McRaptor. We will use this later to provide a better neutral travel search. This PR does not change the existing behaviour - the "one" generalized-cost(c1) McRaptor still works as before. Separate state classes which have the additional criteria is added. So when the McRaptor algorithm is configured the appropriate state and calculators are plugged in. The performance for the existing functionality is unchanged - same memory usage and responstimes.

Generalized-cost, C1 and C2
I have introduced two new "words" to the OTP terminology: c1 and c2. This is criteria-one and criteria-two in the MCRaptor state. We store the computed generalized-cost in c1. In principle we can store any computed criteria we want to use in the travel search in c1 an c2. So in the algorithm implementation the generalized-cost is commonly used, while in the state c1 is used.

Is the C2 automatically included in the pareto-comparason?

NO! To use the c2 in the a compare function must be provided. This will depend on what you want to use the C2 criteria for. The implementation is done in such way that the
computation of c2 and the pareto-set compare function is pluggable. This PR provided functionality for using transit-group-priority as c2. The compare function relax the generalized-cost(c1) comparason if (c2) is different. The c2 is not added as an independent criteria, because that would lead to poor performance. Of, cause doing this do not return all optimal results. It is not possible to compute a sensible heuristics for transit-group-priority. By not having c2 as an independent criteria we avoid the heuristic problem as well.

This is part of #3665. I will add another PR on top of this with:

  • API parameters using transit-group-priority
  • Router config for configuring transit-group-priority based on mode, sub-mode, agency and route.
  • Module tests and more unit tests.
  • More documentation.

Issue

Part of #3665

Unit tests

Many new unit-tests and updated tests

Documentation

TODO

  • Add more documentation (All TODO C2 will be fixed in a later PR). There are still some floating pieces.

t2gran added 24 commits March 23, 2023 11:46
The method is not used from the DefaultRangeRaptorWorker. In stead each strategy
is setting this on the state; Hence the method should be pushed down into the
children of RaptorWorkerState.
The StopArrivalsState implementation mostly delegate to an implementation of BestNumberOfTransfers,
so we can avoid the inheritance, and inject the concrete implementation of BestNumberOfTransfers
where needed instead.
This wul enable us to inject a PatternRide with 2 cost criteria later
Move 'relaxCostAtDestination' from SearchParams to MultiCriteriaRequest.
This is annoying, the information is converted doc inside the code.
 - Add priorityGroupId to RaptorTripPattern
 - Remove DominanceFunction.noop()
 - Extract GeneralizedCostRelaxFunction from RelaxFunction function
 - Name ParetoSet comparators
 - Add relaxC1 and transitPriorityCalculator to MultiCriteriaRequest
 - Cleanup StopArrivalFactories
@t2gran t2gran added !New Feature A functional feature targeting the end user. Entur On Entur Roadmap +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Mar 23, 2023
@t2gran t2gran added this to the 2.3 milestone Mar 23, 2023
@t2gran
t2gran requested a review from a team as a code owner March 23, 2023 12:46
@codecov

codecov Bot commented Mar 23, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 73.64% and project coverage change: +0.07 🎉

Comparison is base (80a261d) 64.03% compared to head (09fcb4b) 64.10%.

❗ Current head 09fcb4b differs from pull request most recent head 853246f. Consider uploading reports for the commit 853246f to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #4996      +/-   ##
=============================================
+ Coverage      64.03%   64.10%   +0.07%     
- Complexity     13566    13694     +128     
=============================================
  Files           1675     1687      +12     
  Lines          66222    66570     +348     
  Branches        7145     7175      +30     
=============================================
+ Hits           42403    42675     +272     
- Misses         21445    21500      +55     
- Partials        2374     2395      +21     
Impacted Files Coverage Δ
...ntripplanner/model/plan/UnknownTransitPathLeg.java 57.69% <0.00%> (ø)
...ripplanner/raptor/api/model/DominanceFunction.java 0.00% <0.00%> (ø)
...ripplanner/raptor/api/model/RaptorTripPattern.java 100.00% <ø> (ø)
...entripplanner/raptor/api/request/DebugRequest.java 25.00% <ø> (ø)
...lanner/raptor/api/request/DebugRequestBuilder.java 67.50% <ø> (ø)
...entripplanner/raptor/api/request/SearchParams.java 66.25% <ø> (+0.36%) ⬆️
...lanner/raptor/api/request/SearchParamsBuilder.java 82.08% <ø> (+2.92%) ⬆️
...ain/java/org/opentripplanner/raptor/path/Path.java 89.10% <0.00%> (+2.57%) ⬆️
...a/org/opentripplanner/raptor/path/PathBuilder.java 97.61% <ø> (ø)
...rangeraptor/debug/AbstractDebugHandlerAdapter.java 59.45% <ø> (ø)
... and 80 more

... and 59 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread src/main/java/org/opentripplanner/raptor/api/model/DominanceFunction.java Outdated
Comment thread src/main/java/org/opentripplanner/raptor/api/model/DominanceFunction.java Outdated
Comment thread src/main/java/org/opentripplanner/raptor/api/model/DominanceFunction.java Outdated
Comment thread src/main/java/org/opentripplanner/raptor/package.md Outdated
@t2gran t2gran mentioned this pull request Mar 24, 2023
8 tasks
@@ -1,3 +1,3 @@
tcId,nTransfers,duration,cost,walkDistance,startTime,endTime,agencies,modes,routes,stops,details
1,0,41m47s,-1,0,13:18:13,14:00:00,,,,,Unknown transit 0tx 41m47s

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you explain why this PR changes the non-C2 results?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is this fix(c7e2032). The minDurationRoutingStrategy now respect the alightSlack if no ConstrainedTransfers exist between to stops. This make the heuristic a bit tighter and the performance better.

The code line is:
c7e2032#diff-13e08433be2742e1e92ec82e4ece37c45dc793afb140b5ec211bf1707bca1908R74

where we before used 0, and now pass in the slack.

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've picked up on a few typos.

vpaturet
vpaturet previously approved these changes Mar 30, 2023
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
@vpaturet
vpaturet self-requested a review March 30, 2023 13:32
@t2gran
t2gran merged commit 6ab6121 into opentripplanner:dev-2.x Mar 31, 2023
@t2gran
t2gran deleted the otp2_c2_part_1 branch March 31, 2023 07:55
t2gran pushed a commit that referenced this pull request Mar 31, 2023
t2gran pushed a commit that referenced this pull request Mar 31, 2023
@juggjones

Copy link
Copy Markdown

(Apologies in advance if this is the wrong place to ask but I couldn't find any documentation on how to use multi-criteria RAPTOR within the docs at all. )

If one wanted to add a criteria to optimize for, for example:
image

For a journey from King's Cross to Moorgate, the two routes evaluated would be:

Route Journey Time Transfers S
King’s Cross , Angel , Old Street , Moorgate 11m 0 26 (10+8+4+4)
King’s Cross , Stratford , TRANSFER , Whitechapel , Moorgate 19m 1 18 (10+2+2+4)

Any ideas on if this can be done with the current implementation?

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 Entur On Entur Roadmap !New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants