-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove VehicleToStopHeuristics
#5381
Conversation
b4b4e24
to
b5d270c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Should we add default time-penalty config before removing this ? That would help people using it, since the system will behave almost the same. |
I think that's a good idea. |
Pull request was converted to draft
b5d270c
to
8c772a3
Compare
6a72769
to
96d0c2d
Compare
src/main/java/org/opentripplanner/routing/api/request/preference/AccessEgressPreferences.java
Show resolved
Hide resolved
46b3db4
to
38014a5
Compare
There was a problem hiding this 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)?
891102d
to
16f21df
Compare
191ee41
to
d06a4db
Compare
VehicleToStopHeuristics
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? |
@t2gran this feature is still in use at Entur. What would be the proper way to update our configuration? |
@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. |
@vesameskanen Are the default settings too aggressive? |
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. |
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. |
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