Baseline duration of carpool trip fix #7206
Conversation
| /** | ||
| * Calculates the additional duration caused by inserting this passenger. | ||
| * Calculates the difference between the total duration when inserting this passenger, | ||
| * and the duration when driving straight from the start to the start of the trip. |
There was a problem hiding this comment.
s/from the start to the start of the trip./from the start to the end
| int dropoffPosition, | ||
| List<GraphPath<State, Edge, Vertex>> routeSegments, | ||
| Duration baselineDuration, | ||
| Duration durationBetweenStartAndStop, |
There was a problem hiding this comment.
Stop is ambiguous here since the intermediate locations the trip will visit are also termed stops. Maybe durationBetweenStartAndEnd or durationBetweenOriginAndDestination?
There was a problem hiding this comment.
I changed to durationBetweenOriginAndDestination
| * | ||
| * @return route from coordinate from to coordinate to, or null if routing function fails | ||
| */ | ||
| private GraphPath<State, Edge, Vertex> routeFromStartToStop( |
There was a problem hiding this comment.
Same here, Stop is ambiguous in this context. Maybe the singular routeSegment to differentiate from the other one that computes multiple segments?
There was a problem hiding this comment.
I changed to routeSegment
| return null; | ||
| } | ||
|
|
||
| Duration durationBetweenStartAndStop = calculateDuration(pathBetweenStartAndStop); |
There was a problem hiding this comment.
Same here about stop being ambiguous.
leonardehrenfried
added
+Sandbox
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #7206 +/- ##
=============================================
+ Coverage 72.07% 72.08% +0.01%
- Complexity 20907 20914 +7
=============================================
Files 2282 2282
Lines 84459 84469 +10
Branches 8433 8434 +1
=============================================
+ Hits 60873 60890 +17
+ Misses 20617 20613 -4
+ Partials 2969 2966 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…eturn the same value given the same paramters
hakoncarlsen
force-pushed
the
baselineDuration_of_carpool_trip_fix
branch
from
4dea270 to
a08e236
Compare
hakoncarlsen
force-pushed
the
baselineDuration_of_carpool_trip_fix
branch
from
a08e236 to
b5061df
Compare
| } | ||
|
|
||
| /** | ||
| * Routes from start to stop |
There was a problem hiding this comment.
Start to stop here too. Please avoid this.
|
|
||
| Duration[] cumulativeDurations = calculateCumulativeDurations(baselineSegments); | ||
|
|
||
| GraphPath<State, Edge, Vertex> pathBetweenStartAndStop = routeSegment( |
There was a problem hiding this comment.
Same here, avoid start and stop
4 participants
Summary
A carpool trip has a maximum deviation time budget, which for now is set to a constant 15 minutes as a temporary value. This is how much longer the driver accepts that the trip will take with extra passengers, compared to how long it would take if the driver did the trip alone.
To check whether the deviation budget was exceeded, the following calculation was used:
additional duration = total duration - baseline duration.
The problem is that the baseline duration is calculated using all stops in the trip, including stops for any previously inserted passengers. This means that the deviation budget could be exceeded for carpooling trips with multiple passengers. This PR fixes this issue.
Unit tests
A test findOptimalInsertion_onDeviationBudgetExceeded_returnsNull is added, in which the deviation budget of a carpooling trip with another passenger is exceeded only after this fix.
A few other tests of InsertionEvaluatorTest are also refractored into using a deterministic routing function, since the previous design using a function which depended on the order in which it was called caused problems. Some of the tests also had to have their durations changed for the tests to pass.