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.

Sign up for GitHub

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

Jump to bottom

Transit group priority - part 2 #5524

Merged
merged 14 commits into from
Dec 18, 2023
Merged

Transit group priority - part 2 #5524

merged 14 commits into from
Dec 18, 2023

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Nov 22, 2023 •
edited
Loading

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 #4999
Fixes most of #3665

Todo

  • Make sure all new classes have unit-tests.
  • 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 bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog labels Nov 22, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Nov 22, 2023
@t2gran t2gran requested a review from a team as a code owner November 22, 2023 12:42
@t2gran t2gran marked this pull request as draft November 22, 2023 12:43
@codecov Codecov
Copy link

codecov bot commented Nov 22, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (00db8f4) 67.24% compared to head (5c9732c) 67.35%.
Report is 86 commits behind head on dev-2.x.

Files Patch % Lines
...n/deletionflagger/NumItinerariesFilterResults.java 30.00% 7 Missing ⚠️
...anner/framework/text/CharacterEscapeFormatter.java 82.85% 5 Missing and 1 partial ⚠️
...m/filterchain/ItineraryListFilterChainBuilder.java 79.16% 4 Missing and 1 partial ⚠️
...oradapter/transit/mappers/RaptorRequestMapper.java 0.00% 5 Missing ⚠️
...ntripplanner/routing/api/request/RouteRequest.java 0.00% 4 Missing and 1 partial ⚠️
.../opentripplanner/service/paging/PagingService.java 93.24% 2 Missing and 3 partials ⚠️
...er/model/plan/paging/cursor/PageCursorFactory.java 85.00% 2 Missing and 1 partial ⚠️
.../transferoptimization/model/OptimizedPathTail.java 75.00% 1 Missing and 2 partials ⚠️
...n/java/org/opentripplanner/framework/lang/Box.java 88.23% 0 Missing and 2 partials ⚠️
...model/plan/paging/cursor/PageCursorSerializer.java 96.22% 1 Missing and 1 partial ⚠️
... and 13 more
Additional details and impacted files 
@@              Coverage Diff              @@
##             dev-2.x    #5524      +/-   ##
=============================================
+ Coverage      67.24%   67.35%   +0.11%     
- Complexity     16087    16164      +77     
=============================================
  Files           1852     1858       +6     
  Lines          70980    71093     +113     
  Branches        7404     7403       -1     
=============================================
+ Hits           47733    47888     +155     
+ Misses         20779    20746      -33     
+ Partials        2468     2459       -9

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

@t2gran
fix: Run relaxTransitPriorityGroup when enabled
a274d50 
Signed-off-by: Thomas Gran <t2gran@gmail.com>
@t2gran
fix: Include c2 in Raptor Path
7a528c2 
Signed-off-by: Thomas Gran <t2gran@gmail.com>
@t2gran
refactor: Remove base priority group
6809267 
All patterns not matching a group is assigned to the base group(id=0).
@t2gran
refactor: Create factory for Raptor test arrivals
0f7d32d 
Signed-off-by: Thomas Gran <t2gran@gmail.com>
@t2gran
refactor: Add transfers to factory for Raptor test arrivals
832c36f 
Signed-off-by: Thomas Gran <t2gran@gmail.com>
@t2gran
refactor: Add transit to factory for Raptor test arrivals
7dbf2de 
Signed-off-by: Thomas Gran <t2gran@gmail.com>
@t2gran
refactor: Add egress to factory for Raptor test arrivals
50aba44 
Signed-off-by: Thomas Gran <t2gran@gmail.com>
@t2gran t2gran force-pushed the otp_transit_priority_part_2 branch 2 times, most recently from cd1f8bd to e2913f0 Compare December 12, 2023 17:23
@t2gran
refactor: Return NOT_SET for c2 if not in use.
d0a4666
@t2gran
refactor: Rename generalizedCost to c1 in Raptor
79f82cd 
We use the term generalizedCost in OTP, but not in Raptor. With the new c2 value, renaming
to c1 inside Raptor make the raptor code more consistent. It also becomes easier to keep
raptor cost apart from the OTP model cost - they are slightly different and the context
is important.
@t2gran
feature: Add c2 value to toStrings for better debugging and improve n…
436a3bf 
…otation.
@t2gran t2gran marked this pull request as ready for review December 13, 2023 17:22
@jtorin jtorin self-requested a review December 14, 2023 12:38
@leonardehrenfried
Copy link
Member

Please resolve the conflicts.

habrahamsson-skanetrafiken
habrahamsson-skanetrafiken requested changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like that this PR differentiaties between c1 and generalizedCost, it makes it easier to see whether you are looking at a raptor cost or a street routing cost.

I've just got a few minor nitpicks and some questions for my own understanding.

src/main/java/org/opentripplanner/raptor/api/path/PathLeg.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/path/Path.java Outdated Show resolved Hide resolved
...java/org/opentripplanner/routing/algorithm/transferoptimization/model/OptimizedPathTail.java Show resolved Hide resolved
...ava/org/opentripplanner/routing/algorithm/transferoptimization/model/TripToTripTransfer.java Show resolved Hide resolved
src/test/java/org/opentripplanner/raptor/_data/stoparrival/BasicPathTestCase.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/api/model/RaptorValueFormatter.java Show resolved Hide resolved
@t2gran @habrahamsson-skanetrafiken
Apply suggestions from code review
62a6aa5 
Co-authored-by: Henrik Abrahamsson <127481124+habrahamsson-skanetrafiken@users.noreply.github.com>
jtorin
jtorin requested changes Dec 15, 2023
View reviewed changes
docs/RouteRequest.md Outdated Show resolved Hide resolved
docs/RouteRequest.md Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/api/model/RaptorValueFormatter.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/api/path/PathStringBuilder.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/routing/api/request/request/TransitRequest.java Outdated Show resolved Hide resolved
...main/java/org/opentripplanner/standalone/config/routerequest/TransitPriorityGroupConfig.java Outdated Show resolved Hide resolved
...main/java/org/opentripplanner/standalone/config/routerequest/TransitPriorityGroupConfig.java Outdated Show resolved Hide resolved
src/test/java/org/opentripplanner/routing/stoptimes/AlternativeLegsTest.java Outdated Show resolved Hide resolved
src/test/java/org/opentripplanner/routing/stoptimes/AlternativeLegsTest.java Outdated Show resolved Hide resolved
@jtorin
Copy link
Contributor

jtorin commented Dec 15, 2023

Just some spelling and grammar fixes.

I agree with @habrahamsson-skanetrafiken, the separation of naming between the domains is nice!

t2gran and others added 2 commits December 17, 2023 23:06
@t2gran
Merge remote-tracking branch 'otp/dev-2.x' into otp_transit_priority_…
3484644 
…part_2
@t2gran @jtorin
Apply suggestions from code review
5c9732c 
Co-authored-by: Johan Torin <jtorin@users.noreply.github.com>
@t2gran t2gran merged commit 5adb29f into opentripplanner:dev-2.x Dec 18, 2023
5 checks passed
@t2gran t2gran deleted the otp_transit_priority_part_2 branch December 18, 2023 15:17
t2gran pushed a commit that referenced this pull request Dec 18, 2023
Bump serialization version id for #5524
4994339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtorin jtorin jtorin approved these changes

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken habrahamsson-skanetrafiken approved these changes

Assignees
No one assigned
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR New Feature Skip Changelog
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.
4 participants
@t2gran @leonardehrenfried @jtorin @habrahamsson-skanetrafiken