Add DRT detour constraints (based on PR #2834) #2997
Conversation
Based on the PR #2834
| @Override | ||
| public Optional<AcceptedDrtRequest> acceptDrtOffer(DrtRequest request, double departureTime, double arrivalTime) { | ||
| double updatedPickupTimeWindow = Math.min(departureTime | ||
| + promisedPickupTimeWindow, request.getLatestStartTime()); |
There was a problem hiding this comment.
Consider a different name than promisedPickupTimeWindow. We usually think of a time window as a [a,b) period, where a and b are time stamps. Here, promisedPickupTimeWindow is a duration and represents a time buffer.
In the drt config group, we use maxAllowedPickupDelay instetad of promisedPickupTimeWindow. I think we should also use this name in this class.
There was a problem hiding this comment.
Also updatedPickupTimeWindow should be renamed. Maybe updatedLatestStartTime?
| * @return maximum ride time | ||
| */ | ||
| static double getMaxRideTime(DrtConfigGroup drtCfg, double unsharedRideTime) { | ||
| return Math.min(unsharedRideTime + drtCfg.maxAbsoluteDetour, drtCfg.maxDetourAlpha * unsharedRideTime + drtCfg.maxDetourBeta); |
There was a problem hiding this comment.
I am not sure if we should use unsharedRideTime + drtCfg.maxAbsoluteDetour in both getMaxTravelTime() and getMaxRideTime().
Imposing this constraint on maxTravelTime implies it is also imposed on maxRideTime, because maxTravelTime >= maxRideTime.
|
|
||
| @Parameter | ||
| @Comment( | ||
| "Defines the maximum delay allowed from the initial scheduled pick up time. Once a estimated pick up time is determined, the DRT optimizer" |
There was a problem hiding this comment.
Maybe rewrite the second sentence in this way?
"Once the initial pickup time is offered, he DRT optimizer must not exceed the promised time by the maxAllowedPickupDelay amount of time."
| @Parameter | ||
| @Comment( | ||
| "Defines the maximum allowed absolute detour based on the unsharedRideTime. A linear combination similar to travel time constrain is used" | ||
| + "This is the ratio part") | ||
| @DecimalMin("1.0") | ||
| public double maxDetourAlpha = Double.POSITIVE_INFINITY; | ||
|
|
||
| @Parameter | ||
| @Comment( | ||
| "Defines the maximum allowed absolute detour based on the unsharedRideTime. A linear combination similar to travel time constrain is used" | ||
| + "This is the constant part") | ||
| @PositiveOrZero | ||
| public double maxDetourBeta = Double.POSITIVE_INFINITY;// [s] |
There was a problem hiding this comment.
These two comments should mention that the detour is computed from the latest promised pickup time and not from the actual pickup time.
I think we should also mention the following somewhere (maybe in MaxDetourInsertionCostCalculator):
By taking into account the latest promised pickup time (instead of the actual pickup time), we simplify the handing of this constraint, because the actual pickup time remains unknown until the moment of picking up the passenger.
BTW. In a simulation setup without congestion, the actual pickup time is always less than or equal to the promised time. With congestion there is no such a guarantee.
There was a problem hiding this comment.
Thanks, @michalmac! I will update the descriptions and comments based on the comments.
|
Thanks @luchengqi7 ! @michalmac would it make sense to consolidate the OfferAcceptor and InsertionCostCalculator to implement the new constraint in the default implementations? The parameters are already part of the default drt config and request anyway |
Good point! Actually, that was the main reason for adding these params to the config (I assume). |
Oh yeah, I will update that! 😀 |
PromisedPickupTimeWindowDrtOfferAcceptor -> MaxDetourOfferAcceptor MaxDetourInsertionCostCalculator -> DefaultInsertionCostCalculator
To make sure the test will be based on the new expected values
Based on Michal's suggestion.
This reverts commit d69429b.
|
@luchengqi7 I reenabled IT tests in taxi and rerun them on my laptop (intel CPU). The problem with CI failures is that the workflows are run on a linux machine with an intel CPU. When runnin on a mac machine with an apple cpu, you get slight differences in scores (output plans). I think, in the long term, we need to change the way how we compare output plans to account for differences between these platforms. |
|
thanks so much @luchengqi7 ! |
This PR is based on the finished version of PR #2834, built based on the latest matsim-lib.
For more details, please refer to the previous PR.