CT2 - Cost on transfer in Raptor#3617
Closed
t2gran wants to merge 35 commits into
Closed
Conversation
This will enforce the scope of some of the variables passed to the transit worker, and simplify the already complex `route()` method.
Raptor calculate a onBoardRelativeCost and a transitArrivalCost. These calculations is done with out reusing any logic. But, when this become problematic when we want to add a "sophisticated" transfer calculation - we do not want to do it twice and the transfer information is lost at the time we calculate the transit stop arrival cost. To solve this we can calculate the cost at board time and reuse this value when calculating the onBoardRelativeCost and a transitArrivalCost.
Jupiter is the new JUnit 5 API
Internal refactor the PatternGuaranteedTransferProvider to make the transfer instance available at the root in the call hierarchy. We will use this to calculate a cost for the transit boarding later.
This is a pure refactor and no functionality is changed. The cost calculation belong to the OTP internal model and should not be part of the Raptor algorithm implementation. This enable us to switch the cost calculation and use OTP model information in the cost calculation without passing the information through Raptor. In the future we would like to extend Raptor to use 2 or more cost criteria, this commit is a step in that direction.
This is discussed in the mailing list: https://groups.google.com/g/opentripplanner-dev/c/IIhkC7-ujEw
All results in tests using the stops in the RaptorTestConstants is translated so the asserts are done on stop names, not stop index. This make the asserts use the same "language" as the test specifications, where we use constants like `STOP_A`.
The name clashes with standard Java class.
Move the generalized-cost formatter to OtpNumberFormat and reuse it. Only print "cent" part if not zero, this make a ton of test a bit easier to read.
From PatternTransferConstraintsProvider
From RaptorTransferConstraintsProvider. Also, try to find better names on a few related methods and fields.
Fix spelling BoarAndAlightTime to BoardAndAlightTime and move to appropriate package. Fix BoardAndAlightTime#toString as well.
Rename waitTimeAdjustedGeneralizedCost to waitTimeOptimizedCost and make transferPriorityCost available on the Itinerary (OTP internal model and TransModel API). These two fields are for debugging the travel search.
The TransferConstraint is extracted into a immutable value-object. To do routing in Raptor the cost calculator only need the value object, while the path builder and itinerary still need the extended transfer information, but we should get rid of this - since it is redundant. Guaranteed and stay-seated transfers are "facilitated" transfers where we ignore board-, transfer-, and alight-slack. We will also add a grace period in the future for these transfers.
Fis reminding places where the rename of Guaranteed to Constrained transfer is missing.
From PathBuilder
The MAX_WAIT_TIME_NOT_SET belong to the TransferConstraint class, not the ConstrainedTransfer class.
- Use getTransferConstraint, not just getConstraint (ConstrainedTransfer) - Remove unused method ConstrainedTransfer#getMaxWaitTime - Remove unused field OptimizedPath#originalPath - Update div JavaDoc - Remove redundant List.copyOf in TransferOptimizedFilterFactory - Improve naming in TransitPathLegSelector, McTransitWorker - Improve tests
t2gran
added
!New Feature
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
The interface is given a long name, while the implementing classes is given relatively short names.
Just logging "System error" as a WARNING and the logging the real cause to ERROR is strange. I removed the WARNING and improved the ERROR.
t2gran
force-pushed
the
otp2_cost_transfer
branch
from
2390786 to
4a19389
Compare
- Support for reduced cost in Raptor for constrained transfers - New Path builder, used in Raptor mappers(FWD & RWS), Optimize transfers and in TestPathBuilder.
Unit tests are added on OptimizedPathTail and OptimizedPath.
Clean up how transfer constraint is added to path toStrings. Make utility methods for adding cost w/unit.
There is an error in the logic in OptimizedPathTail line 195 and the OptimizedPath line 90, when adding the priority cost. In both places a cost was added for path legs witch did not represent a transfer. Since the transfer constraint is on the transit-leg BEFORE the optional transfer, this logic is a bit complicated. This commit fixes this, and refactor the priority cost calculation, so the cost is always positive. Previously the transfer priority cost was calculated in the TransferPriority enum, but part of the information is in the TransferConstraint, so it make sense to move some of the logic up to he TransferConstraint.
This commit add the ability to log the stop id and name as part of the Raptor returned path. This is continent when logging and debugging in Raptor.
t2gran
force-pushed
the
otp2_cost_transfer
branch
from
73dc26b to
727e2fe
Compare
Member
Author
|
GitHub does not seem that most of the commits are merged, the diff is 35 commits and 200 files. I will close this an create a new one. |
Contributor
|
Just for future. It works correctly when you have merged master back to this branch, as Github always creates a new merge commit when merging pull requests. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR clean up the Optimize transfer, path building and add cost calculation for guaranteed and stay-seated transfers during the Raptor routing. It is the base for adding some of the reminding features for constrained transfer support.
A CONSTRAINED transfer is a transfer with extra constraints. These are imported from GTFS transfers.txt and NeTEx Interchanges. Example of constraints are
stay-seated(blocks in GTFS),guaranteed, transfer priority, max-wait-time. This PR only changes the Raptor and post Raptor code, it does not do anything with the import.Review
Most of the commits are simple refacorings, I recommend doing review commit by commit. The las. commit contain almost all feature changes, this is a bit large, but unfortunally it would be time consuming to split it.
Do PR #3619 first, this PR build on that - 3619 have all commits, except the last one - which contains the feature set.
Issue
closes #3478
Unit tests
I have not done any performance testing on this, but with few constrained transfers the performance change should be minimal. Also, the
OTPFeature#TransferConstraintscan be turned off - in witch case the performance should be the same as before.Documentation
All relevant documentation should be up to date, there is very little doc on this in the documentation.
Changelog
TODO Add Changelog item