Remove default accessegress penalty for car modes that use transit #6414
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6414 +/- ##
=============================================
+ Coverage 69.84% 69.86% +0.02%
+ Complexity 18124 18123 -1
=============================================
Files 2069 2069
Lines 77268 77240 -28
Branches 7855 7845 -10
=============================================
- Hits 53965 53964 -1
+ Misses 20546 20524 -22
+ Partials 2757 2752 -5 ☔ View full report in Codecov by Sentry. |
| @@ -171,7 +171,9 @@ private static TimeAndCostPenaltyForEnum<StreetMode> createDefaultCarPenalty() { | |||
| // Add penalty to all car variants with access and/or egress. | |||
There was a problem hiding this comment.
This comment should be updated.
There was a problem hiding this comment.
Updated to the comment Thomas wrote below
| if ( | ||
| it.includesDriving() && (it.accessAllowed() || it.egressAllowed()) && !it.transferAllowed() | ||
| ) { |
There was a problem hiding this comment.
| if ( | |
| it.includesDriving() && (it.accessAllowed() || it.egressAllowed()) && !it.transferAllowed() | |
| ) { | |
| // Apply car-penalty to all car modes allowed in access/egress only. Exclude car modes(CAR) used | |
| // in direct street routing and car modes used when you bring the car with you onto transit. This is | |
| // a bit limited and will not work if we combine car access/egress modes like CAR_TO_PARK with CAR | |
| // in the same search. This is currently not possible, but if we enable this in the future this logic must be | |
| // looked at again. | |
| if ( | |
| it.includesDriving() && (it.accessAllowed() || it.egressAllowed()) && it != CAR | |
| ) { |
There was a problem hiding this comment.
I changed it to this and removed the existing comment
|
I tried, but did not find any issue discussing the issues around penalty on CAR and other street modes. |
|
We discussed it (very lightly) in the original PR: #6302 |
|
Yes I fount it, but I was almost sure there was a issue with a more in dept discussion. The PR was not very helpful, but good to reference it anyway. |
| // Apply car-penalty to all car modes allowed in access/egress only. Exclude car modes(CAR) used | ||
| // in direct street routing and car modes used when you bring the car with you onto transit. This is |
There was a problem hiding this comment.
Pretty sure CAR_TO_PARK and CAR_RENTAL are at least allowed in direct routing and I think they are not excluded now.
There was a problem hiding this comment.
I made a suggestion for the comment. Most of the itinerary filters work fine with this, but comparing transit and direct is not ideal. We do take away the penalty (both time and cost) in these cases, so the comparison is ok - but it is very difficult to understand just lokking at the results/tuning the filters.
The main thing is that MODES used together with transit in the same journey should have a penalty. I do not think the penalty is applied to direct searches, so it might not hurt to have it specified for direct modes, but I think it is confusing if we do.
There was a problem hiding this comment.
I changed the comment to the suggestion by @t2gran
| // Apply car-penalty to all car modes allowed in access/egress only. Exclude car modes(CAR) used | ||
| // in direct street routing and car modes used when you bring the car with you onto transit. This is | ||
| // a bit limited and will not work if we combine car access/egress modes like CAR_TO_PARK with CAR | ||
| // in the same search. This is currently not possible, but if we enable this in the future this logic must be | ||
| // looked at again. |
There was a problem hiding this comment.
| // Apply car-penalty to all car modes allowed in access/egress only. Exclude car modes(CAR) used | |
| // in direct street routing and car modes used when you bring the car with you onto transit. This is | |
| // a bit limited and will not work if we combine car access/egress modes like CAR_TO_PARK with CAR | |
| // in the same search. This is currently not possible, but if we enable this in the future this logic must be | |
| // looked at again. | |
| // Apply car-penalty to all car modes used in access/egress. Car modes(CAR) used in direct street | |
| // routing and car modes used when you bring the car with you onto transit should be excluded. The | |
| // penalty should also be applied to modes used in access, egress AND direct (CAR_TO_PARK and | |
| // CAR_RENTAL). This is not ideal, since we get an unfair comparison in the itinerary filters. We will | |
| // live with this for now, but might revisit it later. |
There was a problem hiding this comment.
I changed the comment to this
Summary
This PR removes the default accessegress penalty for car modes that use transit.
Issue
This is a follow-up PR related to issue #5875
Documentation
Automatically generated changes.