Filter out duplicate-like legs in alternative legs - #4868
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #4868 +/- ##
=============================================
+ Coverage 63.79% 63.91% +0.12%
- Complexity 13437 13504 +67
=============================================
Files 1674 1674
Lines 66047 66165 +118
Branches 7139 7155 +16
=============================================
+ Hits 42132 42287 +155
+ Misses 21527 21492 -35
+ Partials 2388 2386 -2
... and 31 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. |
| """ | ||
| Whether all possible visits to the destination stops parent station should be treated as a new alternative leg. | ||
| If true, only the first visit is used to generate an alternative. | ||
| Otherwise a single trip might yield multiple alternative legs. | ||
| """ | ||
| onlyFirstDestinationStop: Boolean = true |
There was a problem hiding this comment.
Do we ever need to set this to false, do we need the parameter?
There was a problem hiding this comment.
I don't think there is any need for the parameter at least at the moment but I thought that it might be good to allow potential other users choose what kind of behavior they want from the query. I would be ok with removing the parameter though and only adding it if someone found an actual need for it
| .iterate(stops.size() - 1, i -> i - 1) | ||
| .limit(stops.size()) | ||
| .filter(i -> destinations.contains(stops.get(i)) && tripPattern.canAlight(i)) | ||
| .limit(onlyFirstDestinationStop ? 1 : destinations.size()) |
There was a problem hiding this comment.
I think it would be enough to have .limit(1) here and other changes in this pr can be removed as this doesn't need to be parametrized, right?
There was a problem hiding this comment.
That is what I would have done but it seemed that the cartesian product of all possible origin and destination stops was written intentionally without the limit so I wanted to keep that option available as well. The parametrization was removed from the LegacyGraphQL query but I'm not sure if we want to replace the original functionality with this as well
There was a problem hiding this comment.
I would guess that this behaviour of including the same trip more than once is never desired but I haven't checked what are all the use cases this code is used for. Maybe @hannesj knows if it would be fine just to have the hardcoded limit(1).
There was a problem hiding this comment.
We need to create two alternative legs for the Oslo t-bane line 5 if the trip is somewhere between Ullevål and Töyen, but only one trip per boarding.
I think it would be nice to have a unit test for behaviour on line like this.
There was a problem hiding this comment.
Ok, so if I understood correctly, the original behavior cannot be removed with the hard limit as it is needed for generating the two alternatives at e.g. Ullevål. I think the current changes should only give one trip as I added a distinct() after .filter(tripPatternPredicate). Otherwise, the second visit to a stop could duplicate some of the trip patterns that would be used later on in the search. Does something else still need to be done apart from adding a unit test?
There was a problem hiding this comment.
We could first gather all possible boarding positions, then generate the shortest leg from each of them, and then grouping by the alighting position and taking the shortest leg for each group.
optionsome
added
!Bug
|
Hannes says
|
|
Please fix the unit tests. |
|
I think the added test covers the Helsinki airport train use case but not necessarily the more complex route 5 example from hannes in #4868 (comment) |
|
We should probably add a test for a trip that goes through stops A -> B -> C -> D -> B -> -> C -> D -> E. If you search for alternative legs from B to C you should get the legs from the stop sequence (index starts at 0) 1 -> 2 and 4 -> 5. However, if you search for legs from B to E you should only get the later stop sequence 4 -> 7. |
| boolean exactOriginStop, | ||
| boolean exactDestinationStop | ||
| boolean exactDestinationStop, | ||
| boolean onlyFirstDestinationStop |
There was a problem hiding this comment.
Delete onlyFirstDestinationStop, not in use.
| @@ -83,7 +84,8 @@ public static List<ScheduledTransitLeg> getAlternativeLegs( | |||
| boolean searchBackward, | |||
There was a problem hiding this comment.
I f you would be kind and rename this I would appreciate it. searchBackward is confusing - to me this indicate that we search from the destination towards the origin - we use search forward/reverse for this in other places of the code. I suggest using includeDepartBefore.
| .min() | ||
| .stream() |
There was a problem hiding this comment.
// Create a cartesian product of the pairs
This is not finding the cartesian product any more, update comment
Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>
6 participants
Summary
This pull request adds an option to filter out duplicate legs in alternative legs. This option is then used in the LegacyGraphQL API nextLegs query and in similar query in the TransmodelGraphQL API.
Issue
In Helsinki, there are two train services that depart and arrive at the same station. Also the second and next to last station in the stop sequence are the same. If a leg starts from the original departure station and ends at the second station, the alternative legs feature also returns alternatives that take the long route to the destination, i.e. instead of taking the train for one stop, one should ride almost the whole route. While these alternatives are technically valid options, they may cause confusion as two or more alternatives look the same.
Unit tests
The outputs of the alternative legs feature have been tested manually. All existing tests pass.