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

Remove transit with higher cost than best on-street itinerary filter #5222

Conversation

vesameskanen
Copy link
Contributor

Summary

RemoveTransitWithHigherCostThanBestOnStreetOnly filter currently removes all transit itineraries, if their cost is any higher than the best of non-transit itineraries. This is bad because a citybike itinerary can erase all equally fast transit itineraries. This is quite anti-multimodal behavior.

This PR adds a similar two parameter cost limit function as other filters which compare itineraries use.

To avoid appearance of silly more walk + transit vs. plain walk itineraries, hard limit is still applied to walk street mode.

Documentation

Updated to describe how new filtering logic works

Unit tests

Unit tests updated.

Softer cost limit function based filtering will sometimes preserve transit itineraries,
which contain more walking than a plain walk itinerary. Therefore, apply harder
filtering logic wrt. walking.
@vesameskanen vesameskanen requested a review from a team as a code owner July 6, 2023 11:27
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage: 94.02% and project coverage change: +0.18% 🎉

Comparison is base (79d1cbc) 66.27% compared to head (200ed4e) 66.46%.
Report is 115 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5222      +/-   ##
=============================================
+ Coverage      66.27%   66.46%   +0.18%     
- Complexity     15171    15225      +54     
=============================================
  Files           1789     1787       -2     
  Lines          69275    69296      +21     
  Branches        7351     7314      -37     
=============================================
+ Hits           45910    46055     +145     
+ Misses         20881    20763     -118     
+ Partials        2484     2478       -6     
Files Changed Coverage Δ
...request/preference/ItineraryFilterPreferences.java 92.15% <77.77%> (-2.66%) ⬇️
...m/filterchain/ItineraryListFilterChainBuilder.java 90.00% <100.00%> (+0.34%) ⬆️
...g/algorithm/filterchain/RoutingErrorsAttacher.java 97.14% <100.00%> (+0.71%) ⬆️
...agger/RemoveTransitIfStreetOnlyIsBetterFilter.java 100.00% <100.00%> (ø)
...nflagger/RemoveTransitIfWalkingIsBetterFilter.java 100.00% <100.00%> (ø)
...rithm/mapping/RouteRequestToFilterChainMapper.java 78.43% <100.00%> (+0.88%) ⬆️
...ne/config/routerequest/ItineraryFiltersConfig.java 99.07% <100.00%> (+0.05%) ⬆️

... and 59 files with indirect coverage changes

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

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.

I think this is a good improvement but I would like someone from Entur to take a look as they are they are the main developers/user of these filters.

@vesameskanen
Copy link
Contributor Author

vesameskanen commented Jul 10, 2023

New cost function is in use here:

https://reittiopas.hsl.fi/reitti/Lapinrinne%202%2C%20Helsinki%3A%3A60.16571397446897%2C24.927077293396/Merikasarminkatu%2012%20F%2C%20Helsinki%3A%3A60.16777953071638%2C24.975668191909794?setTime=true&time=1692271080

In addition to perfectly matching biking between two stations, also plain transit and citybike + transit itineraries are available.

@leonardehrenfried
Copy link
Member

That URL leads to a 400 - could you please correct it?

@vesameskanen
Copy link
Contributor Author

Link to example search above fixed.

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Jul 10, 2023

image

Indeed a very nice variety.

@leonardehrenfried
Copy link
Member

This branch has conflicts

@leonardehrenfried leonardehrenfried requested a review from t2gran July 11, 2023 08:33
@leonardehrenfried
Copy link
Member

Can you please resolve the conflicts?

@t2gran t2gran added this to the 2.4 (next release) milestone Jul 18, 2023
@t2gran
Copy link
Member

t2gran commented Jul 19, 2023

Does this filter do two things now?

  • Filter on a hard limit compared with walking
  • Filter based on a function with street itinerary

If, so the filter should be split into two filters, it should be possible to configure these things separate. An alternative is to make one filter witch is configurable with respect to matching street modes, then the same filter can be used for different cases(walk, walk+bicycle, walk+car ...).

@vesameskanen
Copy link
Contributor Author

Right, it indeed applies two clearly different logic. I can split it if necessary.

@t2gran
Copy link
Member

t2gran commented Jul 20, 2023

I think splitting it is the easiest way to solve it and also "good enough". We discussed it at todays meeting, where I tried to explain the more "generic way" to solve it, but I think it is overkill.

@t2gran t2gran modified the milestones: 2.4, 2.5 (next release) Sep 13, 2023
Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

Some names and doc needs to be fixed

…n/ItineraryListFilterChainBuilder.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
vesameskanen and others added 9 commits September 14, 2023 15:03
…n/deletionflagger/RemoveTransitIfWalkingIsBetterFilter.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ce/ItineraryFilterPreferences.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ce/ItineraryFilterPreferences.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ce/ItineraryFilterPreferences.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ce/ItineraryFilterPreferences.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ce/ItineraryFilterPreferences.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ce/ItineraryFilterPreferences.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…st/ItineraryFiltersConfig.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
@t2gran t2gran changed the title Add cost function to RemoveTransitWithHigherCostThanBestOnStreetOnly filter Remove transit with higher cost than best on-street itinerary filter Sep 14, 2023
…n/deletionflagger/RemoveTransitIfWalkingIsBetterFilter.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
t2gran
t2gran previously approved these changes Sep 14, 2023
@leonardehrenfried
Copy link
Member

You need to format the code.

@leonardehrenfried leonardehrenfried merged commit 429582e into opentripplanner:dev-2.x Sep 14, 2023
t2gran pushed a commit that referenced this pull request Sep 14, 2023
@optionsome optionsome deleted the removetransitifstreetonlyisbetterfilter-costfunc branch September 14, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants