Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transit group priority - Part 1 #4999

Merged
merged 20 commits into from
Nov 22, 2023

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Mar 24, 2023

Summary

Transit group priority enable routing search where the transit services can be grouped and given mutual priority over other groups. By "mutual priority" we mean that both groups are is prioritised and end up in the result. A relax function is used to say how much worse a group may be before it is dropped. Within the group, only the best options are kept.

Issue

This build on top of PR #4996.
Fixes most of #3665

Unit tests

TODO

  • Add module tests for c2 and run all relevant module test with c2 enabled (no used).
  • Make sure all new classes have unit-tests.
  • Support for grouping on Route mode (witch may be different from Trip mode/subMode)
  • Fix issue reluctanceForMode uses route mode, not Trip/pattern #5248
  • Copy and log c2 values to Path, PathLeg and Itinerary+Leg for better debugging.
  • Heuristics - this is needed by both pass-through and this.
  • Cleanup - filtering, make this filters API POJOs not Filters. This will allow us to reuse it in the API and apply it more than one place.

Documentation

TODO

  • Write documentation for this feature

@t2gran t2gran added New Feature Documentation Entur test This is currently being tested at Entur bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Config Change labels Mar 24, 2023
@t2gran t2gran added this to the 2.3 milestone Mar 24, 2023
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Attention: 193 lines in your changes are missing coverage. Please review.

Comparison is base (428970b) 66.97% compared to head (5083681) 67.06%.
Report is 46 commits behind head on dev-2.x.

Files Patch % Lines
...er/transit/model/timetable/ScheduledTripTimes.java 55.88% 29 Missing and 1 partial ⚠️
...onfig/routerequest/TransitPriorityGroupConfig.java 52.63% 18 Missing ⚠️
...est/request/filter/TransitPriorityGroupSelect.java 74.54% 9 Missing and 5 partials ⚠️
...planner/routing/service/DefaultRoutingService.java 36.84% 11 Missing and 1 partial ⚠️
.../timetable/StopTimeToScheduledTripTimesMapper.java 83.33% 11 Missing and 1 partial ⚠️
...tripplanner/transit/model/timetable/TripTimes.java 82.08% 11 Missing and 1 partial ⚠️
...er/ext/transmodelapi/model/plan/RelaxCostType.java 63.33% 11 Missing ⚠️
...planner/api/common/RequestToPreferencesMapper.java 8.33% 10 Missing and 1 partial ⚠️
...radapter/transit/request/PriorityGroupMatcher.java 83.87% 8 Missing and 2 partials ⚠️
...entripplanner/raptor/api/request/SearchParams.java 0.00% 9 Missing ⚠️
... and 21 more
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #4999      +/-   ##
=============================================
+ Coverage      66.97%   67.06%   +0.09%     
- Complexity     15821    15978     +157     
=============================================
  Files           1833     1847      +14     
  Lines          70599    71067     +468     
  Branches        7410     7448      +38     
=============================================
+ Hits           47281    47664     +383     
- Misses         20845    20917      +72     
- Partials        2473     2486      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t2gran
Copy link
Member Author

t2gran commented Mar 24, 2023

I tested all test cases in issue #3665:

Test Results

Case 1

  • Rleaxing by 1.1x+2140 return VY146 13:19 - 15:32 (Dominated by F6 Train 13:39 - 15:00)
  • Bus not found: VY ekspressbuss depart 12:20 from Otta and arrive at Lillehammer 13:52

Case 2

  • Fixed by relaxTransitPriorityGroup=1.1x+4030

Case 3

  • Test case is not valid, trips does not overlap in time.

Case 4

  • Fixed by relaxTransitPriorityGroup=1.2x+6460

Case 5

  • Same route diffrent patterns?

Case 6
This is difficult, but

  • R65 Shown with relaxTransitPriorityGroup=1.0x+1530 and local-tradic in separate pri.group(index 1 not 0)

Case 7

  • Test case description need to be clearified. Strange results VY 146 is shown without relax, but not when relaxTransitPriorityGroup=1.1x+3620

Case 8

  • Is possible solved by adding Røros banen as a priority-group

Case 9

  • This is not about neutral trip planning, this is about optimal place to transfer

Case 10

  • This only possible to support by doing 2 trip calls, one where riding train have a higher cost than waiting.

Case 11
Fix by: relaxTransitPriorityGroup=1.0x+213

Case 12
Fix by: relaxTransitPriorityGroup=1.05x+710

@t2gran t2gran force-pushed the otp2_c2_and_relaxed_mc_criteria branch 2 times, most recently from 72a0ad5 to 38a0c34 Compare April 13, 2023 22:44
@t2gran t2gran modified the milestones: 2.3, 2.4 Apr 24, 2023
@t2gran t2gran removed the Entur test This is currently being tested at Entur label Jun 6, 2023
@t2gran t2gran modified the milestones: 2.4, 2.5 (next release) Sep 13, 2023
@t2gran t2gran force-pushed the otp2_c2_and_relaxed_mc_criteria branch from 38a0c34 to 04f721d Compare November 3, 2023 17:07
@t2gran t2gran changed the title Transit group priority Transit group priority - Part 1 Nov 3, 2023
@t2gran t2gran marked this pull request as ready for review November 3, 2023 17:08
@t2gran t2gran requested a review from a team as a code owner November 3, 2023 17:08
@t2gran
Copy link
Member Author

t2gran commented Nov 3, 2023

I want to merge this to avoid falling to fare behind, I will make a second PR - Part 2 to fix the reminding tasks.

@leonardehrenfried
Copy link
Member

Please format.

@t2gran t2gran requested a review from jtorin November 17, 2023 12:15
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

There are a few tiny suggestions.

Co-authored-by: Johan Torin <jtorin@users.noreply.github.com>
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
@t2gran t2gran force-pushed the otp2_c2_and_relaxed_mc_criteria branch 2 times, most recently from 61128c8 to db846ce Compare November 21, 2023 22:07
@t2gran t2gran force-pushed the otp2_c2_and_relaxed_mc_criteria branch from db846ce to f0bf669 Compare November 22, 2023 09:13
@leonardehrenfried
Copy link
Member

Please resolve the conflicts.

…c_criteria

# Conflicts:
#	src/main/java/org/opentripplanner/raptor/api/request/SearchParams.java
@t2gran t2gran force-pushed the otp2_c2_and_relaxed_mc_criteria branch from 2ef7013 to 5083681 Compare November 22, 2023 12:12
@t2gran t2gran mentioned this pull request Nov 22, 2023
6 tasks
@t2gran t2gran requested a review from jtorin November 22, 2023 13:53
Copy link
Contributor

@jtorin jtorin left a comment

Choose a reason for hiding this comment

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

Found some minor question-marks.

private final RoutingTripPattern railA = routeA.getTripPattern().getRoutingTripPattern();
private final RoutingTripPattern busB = routeB.getTripPattern().getRoutingTripPattern();
private final RoutingTripPattern railC = routeC.getTripPattern().getRoutingTripPattern();
private final RoutingTripPattern ferryC = routeD.getTripPattern().getRoutingTripPattern();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be ferryD ?

Comment on lines +25 to +26
private final TripPattern bus = b1.getTripPattern();
private final TripPattern ferry = f1.getTripPattern();
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't matter that much, but maybe these two should be bus1 and ferry1 to be consistent. Or just rail.

@t2gran t2gran merged commit 043fb0b into opentripplanner:dev-2.x Nov 22, 2023
5 checks passed
@t2gran t2gran deleted the otp2_c2_and_relaxed_mc_criteria branch November 22, 2023 17:07
t2gran pushed a commit that referenced this pull request Nov 22, 2023
t2gran pushed a commit that referenced this pull request Nov 22, 2023
@t2gran
Copy link
Member Author

t2gran commented Nov 22, 2023

@jtorin I will cleanup the test in the next PR...

t2gran pushed a commit to entur/OpenTripPlanner that referenced this pull request Nov 23, 2023
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 Config Change Documentation New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants