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 priority - part 3 #5583

Merged
merged 15 commits into from
Dec 21, 2023
Merged

Transit priority - part 3 #5583

merged 15 commits into from
Dec 21, 2023

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Dec 18, 2023

Summary

Fix bug in pareto-function in destination arrival in Raptor. This also include tests and some cleanup.

Issue

Part of #3665

Unit tests

✅ Both unit tests and module tests are added for the failing destination pareto-comparason.

Documentation

🟥 This feature is still WIP, and we are not going to document it before we have something we believe is working properly. But, with this PR it should be ready for testing it out.

Changelog

✅ This should be included in the changelog.

Bumping the serialization version id

🟥 Not needed.

@t2gran t2gran added the Bug label Dec 18, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Dec 18, 2023
@t2gran t2gran requested a review from a team as a code owner December 18, 2023 18:08
@codecov Codecov
Copy link

codecov bot commented Dec 18, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (4230fd3) 67.35% compared to head (b00c648) 67.45%.
Report is 84 commits behind head on dev-2.x.

Files Patch % Lines
.../raptor/rangeraptor/path/configure/PathConfig.java 82.35% 2 Missing and 1 partial ⚠️
...java/org/opentripplanner/model/plan/Itinerary.java 75.00% 2 Missing ⚠️
...r/multicriteria/configure/McRangeRaptorConfig.java 77.77% 1 Missing and 1 partial ⚠️
...tor/rangeraptor/path/PathParetoSetComparators.java 92.59% 1 Missing and 1 partial ⚠️
...algorithm/mapping/RaptorPathToItineraryMapper.java 0.00% 1 Missing and 1 partial ⚠️
...a/org/opentripplanner/raptor/path/PathBuilder.java 0.00% 0 Missing and 1 partial ⚠️
.../raptor/rangeraptor/internalapi/ParetoSetCost.java 87.50% 1 Missing ⚠️
...va/org/opentripplanner/raptor/spi/UnknownPath.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             dev-2.x    #5583      +/-   ##
=============================================
+ Coverage      67.35%   67.45%   +0.09%     
- Complexity     16162    16186      +24     
=============================================
  Files           1858     1861       +3     
  Lines          71093    71161      +68     
  Branches        7403     7397       -6     
=============================================
+ Hits           47888    48001     +113     
+ Misses         20745    20673      -72     
- Partials        2460     2487      +27

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

@jtorin jtorin self-requested a review December 19, 2023 10:06
@leonardehrenfried leonardehrenfried marked this pull request as draft December 19, 2023 10:29
@t2gran
fix: Use *and*, not *or* when joining matchers in transit-priority-gr…
2639213 
…oups.
@t2gran
feature: Make base transit-priority-group a group by its own.
591f769 
There are two ways we can treat the base (local-traffic) transit priority group.
We can treat is a separate group, equal to all other groups in behaviour - or we
can treat is special and not add it to the comparason vector. Let B be the base
and G be concreate group. Then: (B) dominates (G), (G) dominates (B), (B)
dominates (BG), but (G) does not dominate (BG). With other words, paths with only
agency X(group G) is not given an advantige in the routing over paths with a
combination of agency X(group G) and local trafic (group B). This commit turn
this advanced feature OFF for now.
jtorin
jtorin requested changes Dec 19, 2023
View reviewed changes
src/main/java/org/opentripplanner/raptor/rangeraptor/context/SearchContext.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/rangeraptor/internalapi/ParetoSetCost.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/rangeraptor/internalapi/ParetoSetTime.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/rangeraptor/path/PathParetoSetComparators.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/rangeraptor/path/PathParetoSetComparators.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/raptor/rangeraptor/path/PathParetoSetComparators.java Show resolved Hide resolved
src/test/java/org/opentripplanner/raptor/moduletests/K02_TransitPriorityDestinationTest.java Outdated Show resolved Hide resolved
src/test/java/org/opentripplanner/raptor/moduletests/K01_TransitPriorityTest.java Outdated Show resolved Hide resolved
@t2gran t2gran marked this pull request as ready for review December 19, 2023 16:24
@t2gran @jtorin
Apply suggestions from code review
275a2f2 
Co-authored-by: Johan Torin <jtorin@users.noreply.github.com>
@t2gran
Copy link
Member Author

t2gran commented Dec 19, 2023

Sorry for the mess with small commits and the rebase/squash in the end, but GitHub refused to apply some of the suggestions - but did not tell me which one ...

@t2gran t2gran requested a review from jtorin December 19, 2023 17:03
@t2gran t2gran added the Entur test This is currently being tested at Entur label Dec 19, 2023
leonardehrenfried
leonardehrenfried requested changes Dec 20, 2023
View reviewed changes
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.

Please have a look at my requests.

@jtorin
Copy link
Contributor

jtorin commented Dec 20, 2023

Found some minor spelling nits while reviewing the latest changes.

t2gran and others added 3 commits December 21, 2023 01:03
@t2gran @leonardehrenfried @jtorin
Apply suggestions from code review
9f221b3 
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
Co-authored-by: Johan Torin <jtorin@users.noreply.github.com>
@t2gran @leonardehrenfried
Apply suggestions from code review
f3adc14 
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
@t2gran
Apply suggestions from code review
8ed3825
@t2gran t2gran mentioned this pull request Dec 21, 2023
Merged
jtorin
jtorin previously approved these changes Dec 21, 2023
View reviewed changes
@jtorin
Copy link
Contributor

jtorin commented Dec 21, 2023

schema.graphql differ from the expected document 🙄

Please regenerate.

@t2gran
Copy link
Member Author

t2gran commented Dec 21, 2023

I will merge this with just @leonardehrenfried approval. Since @jtorin already approved and it was just a technical problem with the generated schema.

@t2gran t2gran merged commit 4b022db into opentripplanner:dev-2.x Dec 21, 2023
5 checks passed
@t2gran t2gran deleted the otp_transit_priority_part_3 branch December 21, 2023 16:48
t2gran pushed a commit that referenced this pull request Dec 21, 2023
Add changelog entry for #5583 [ci skip]
3c93bb0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonardehrenfried leonardehrenfried leonardehrenfried approved these changes

@jtorin jtorin jtorin left review comments

Assignees
No one assigned
Labels
Bug Entur test This is currently being tested at Entur New Feature
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.
3 participants
@t2gran @jtorin @leonardehrenfried