Skip to content

Remove VehicleToStopHeuristics - #5381

Merged
leonardehrenfried merged 12 commits into
opentripplanner:dev-2.xfrom
leonardehrenfried:remove-heuristics
Feb 7, 2024
Merged

Remove VehicleToStopHeuristics#5381
leonardehrenfried merged 12 commits into
opentripplanner:dev-2.xfrom
leonardehrenfried:remove-heuristics

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Sep 28, 2023

Copy link
Copy Markdown
Member

Summary

I received several reports that this sandbox feature, which I wrote quite a while ago, was returning very strange results.

Since we have a much better way to control the access/egress with either a maxStopCount or the penalty, rather than investigating why it fails, I decided to remove it

Fixes #5337

@leonardehrenfried leonardehrenfried added +Sandbox This will be implemented as a Sandbox feature !Technical Debt Improve code quality, no functional changes. +Config Change This PR might require the configuration to be updated. labels Sep 28, 2023
@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner September 28, 2023 09:06
@codecov

codecov Bot commented Sep 28, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (14aca8a) 67.54% compared to head (d06a4db) 67.63%.
Report is 1 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5381      +/-   ##
=============================================
+ Coverage      67.54%   67.63%   +0.09%     
- Complexity     16293    16297       +4     
=============================================
  Files           1888     1886       -2     
  Lines          71609    71570      -39     
  Branches        7405     7390      -15     
=============================================
+ Hits           48365    48407      +42     
+ Misses         20741    20661      -80     
+ Partials        2503     2502       -1     

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

@vesameskanen
vesameskanen self-requested a review September 28, 2023 11:50
vesameskanen
vesameskanen previously approved these changes Sep 28, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Sep 28, 2023
@leonardehrenfried
leonardehrenfried requested review from t2gran and vpaturet and removed request for t2gran September 28, 2023 14:29
@t2gran

t2gran commented Oct 2, 2023

Copy link
Copy Markdown
Member

Should we add default time-penalty config before removing this ? That would help people using it, since the system will behave almost the same.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

I think that's a good idea.

@leonardehrenfried
leonardehrenfried marked this pull request as draft October 12, 2023 08:28
auto-merge was automatically disabled October 12, 2023 08:29

Pull request was converted to draft

@leonardehrenfried
leonardehrenfried marked this pull request as ready for review October 16, 2023 16:24
@leonardehrenfried
leonardehrenfried force-pushed the remove-heuristics branch 2 times, most recently from 6a72769 to 96d0c2d Compare October 16, 2023 20:36
@leonardehrenfried
leonardehrenfried marked this pull request as draft October 16, 2023 21:30
@leonardehrenfried
leonardehrenfried marked this pull request as ready for review October 17, 2023 07:36
@leonardehrenfried
leonardehrenfried marked this pull request as draft October 17, 2023 09:35
@leonardehrenfried
leonardehrenfried marked this pull request as ready for review October 20, 2023 09:31

@vesameskanen vesameskanen left a comment

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.

This is otherwise OK, but the new fairly high default penalties create a side effect, which is sort of a known problem: penalty is applied only when the defined street modes are changed to a transit mode. This causes a strange bias, because transit gets penalty whereas another street mode does not. So, walking several kilometers after a car leg is cheaper than taking much faster transit connection. See attached picture. This needs a solution - any ideas (other than configuring penalties back to zero)?

park-and-walk

@leonardehrenfried
leonardehrenfried marked this pull request as ready for review February 1, 2024 13:21
@leonardehrenfried leonardehrenfried changed the title Remove VehicleToStopHeuristics Remove VehicleToStopHeuristics Feb 1, 2024
@leonardehrenfried

Copy link
Copy Markdown
Member Author

This PR has come back to life after a long sleep. And I fixed the unfair advantage of direct results in #5483 so this is ready for review again.

@vesameskanen @vpaturet Can you take a look at it?

@vpaturet

vpaturet commented Feb 5, 2024

Copy link
Copy Markdown
Contributor

@t2gran this feature is still in use at Entur. What would be the proper way to update our configuration?

@leonardehrenfried

leonardehrenfried commented Feb 5, 2024

Copy link
Copy Markdown
Member Author

@vpaturet This PR now sets a default accessEgress penalty for the car modes. They will be good choices for the majority of deployments, including Entur, so you don't need to do anything.

With your expertise, however, you might want to finetune the values.

@t2gran

t2gran commented Feb 5, 2024

Copy link
Copy Markdown
Member

@vesameskanen Are the default settings too aggressive?

@vesameskanen

Copy link
Copy Markdown
Contributor

The default penalty values look high, but current park&ride itineraries seem to be sensible. Our service makes a separate search for plain park & ride, with a high car penalty value of 10.

@vesameskanen

Copy link
Copy Markdown
Contributor

I tested much lower penalty (5m + 1.2t, 1.2). That seems to add variation to itineraries but also adds more car driving. Finding the optimal level is not easy. We will probably use the default values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Config Change This PR might require the configuration to be updated. +Sandbox This will be implemented as a Sandbox feature !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improving Park + Ride results in areas with poor transit or parking lot coverage

4 participants