Skip to content

Include waiting time at beginning and end in TransitGeneralizedCostFilter - #4188

Merged
hannesj merged 4 commits into
opentripplanner:dev-2.xfrom
entur:otp2_wait_time_filter
Jul 27, 2022
Merged

Include waiting time at beginning and end in TransitGeneralizedCostFilter#4188
hannesj merged 4 commits into
opentripplanner:dev-2.xfrom
entur:otp2_wait_time_filter

Conversation

@hannesj

@hannesj hannesj commented May 31, 2022

Copy link
Copy Markdown
Contributor

Summary

This PR improves filtering done in TransitGeneralizedCostFilter, in order to be able to have a tighter value for the filtering function, meaning that we get less worse results close in time, but more results far away from the best itinerary.

Issue

Fixes #4167

Unit tests

Updated the test with no cost for wait time, and some cost for wait time.

@hannesj hannesj added !Improvement A functional improvement or micro feature Entur Test This is currently being tested at Entur labels May 31, 2022
@hannesj hannesj added this to the 2.2 milestone May 31, 2022
@hannesj
hannesj requested a review from a team as a code owner May 31, 2022 07:40
@t2gran
t2gran requested review from MikaelJarfors and t2gran May 31, 2022 09:46
* <li>To set an absolute value(3000) use: {@code 3000 + 0x}
* </ul>
* The default is {@code 3600 + 2x} - 1 hours plus 2 times the lowest cost.
* The default is {@code 900 + 1.5x} - 15 minutes plus 1.5 times the lowest cost.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we changing default value now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The introduction of the intervalRelaxFactor make the function work different than before, so it make sence to change the defaults as well.

@leonardehrenfried

Copy link
Copy Markdown
Member

I tested this today and can confirm that it solves the problem I described at #4167 (comment)

image

@hannesj
hannesj force-pushed the otp2_wait_time_filter branch from 13ad1af to f94ed77 Compare July 26, 2022 14:09
@hannesj
hannesj requested a review from t2gran July 26, 2022 14:09
@codecov-commenter

codecov-commenter commented Jul 26, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4188 (85c93d5) into dev-2.x (589f3c1) will increase coverage by 0.07%.
The diff coverage is 70.90%.

❗ Current head 85c93d5 differs from pull request most recent head ea306cc. Consider uploading reports for the commit ea306cc to get more accurate results

@@              Coverage Diff              @@
##             dev-2.x    #4188      +/-   ##
=============================================
+ Coverage      57.06%   57.14%   +0.07%     
- Complexity     10747    10784      +37     
=============================================
  Files           1430     1430              
  Lines          58032    58116      +84     
  Branches        6720     6729       +9     
=============================================
+ Hits           33117    33211      +94     
+ Misses         22906    22895      -11     
- Partials        2009     2010       +1     
Impacted Files Coverage Δ
...er/ext/transmodelapi/TransmodelGraphQLPlanner.java 0.00% <ø> (ø)
...lanner/ext/transmodelapi/model/plan/TripQuery.java 99.55% <ø> (-0.02%) ⬇️
...opentripplanner/standalone/config/NodeAdapter.java 87.50% <0.00%> (-0.40%) ⬇️
...nner/standalone/config/ItineraryFiltersMapper.java 11.11% <7.69%> (-2.23%) ⬇️
...modelapi/model/plan/ItineraryFiltersInputType.java 78.20% <84.21%> (+2.39%) ⬆️
...m/filterchain/ItineraryListFilterChainBuilder.java 90.81% <100.00%> (ø)
...rchain/api/TransitGeneralizedCostFilterParams.java 100.00% <100.00%> (ø)
.../deletionflagger/TransitGeneralizedCostFilter.java 100.00% <100.00%> (+6.25%) ⬆️
...routing/api/request/ItineraryFilterParameters.java 100.00% <100.00%> (ø)
...anner/standalone/configure/OTPAppConstruction.java 14.75% <0.00%> (-3.25%) ⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 589f3c1...ea306cc. Read the comment docs.

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested this and it indeed improves the cases where the best route is after the latest-departure-time.

Before

Screenshot from 2022-07-27 10-16-46

After

Screenshot from 2022-07-27 10-17-52

Comment thread docs/RouterConfiguration.md Outdated
Co-authored-by: Thomas Gran <t2gran@gmail.com>
@hannesj
hannesj force-pushed the otp2_wait_time_filter branch from 85c93d5 to ea306cc Compare July 27, 2022 09:36
Comment thread src/main/java/org/opentripplanner/standalone/config/ItineraryFiltersMapper.java Outdated
hannesj and others added 2 commits July 27, 2022 12:02
Co-authored-by: Thomas Gran <t2gran@gmail.com>
@hannesj
hannesj merged commit ef62662 into opentripplanner:dev-2.x Jul 27, 2022
@hannesj
hannesj deleted the otp2_wait_time_filter branch July 27, 2022 10:46
t2gran pushed a commit that referenced this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Entur Test This is currently being tested at Entur !Improvement A functional improvement or micro feature WhereIsMyTransport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TransitGeneralizedCostFilter should consider time between itineraries when filtering

5 participants