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 VehicleToStopHeuristics #5381

Merged

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Sep 28, 2023

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

@codecov
Copy link

codecov bot commented Sep 28, 2023

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
Copy link
Member

t2gran commented Oct 2, 2023

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
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
Copy link
Contributor

@vesameskanen vesameskanen left a comment

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
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
Copy link
Contributor

vpaturet commented Feb 5, 2024

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

@leonardehrenfried
Copy link
Member Author

leonardehrenfried commented Feb 5, 2024

@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
Copy link
Member

t2gran commented Feb 5, 2024

@vesameskanen Are the default settings too aggressive?

@vesameskanen
Copy link
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
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.

@leonardehrenfried leonardehrenfried merged commit d79bb93 into opentripplanner:dev-2.x Feb 7, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the remove-heuristics branch February 7, 2024 09:05
t2gran pushed a commit that referenced this pull request Feb 7, 2024
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.

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