Repository navigation
Add filter for minimum bike parking distance - #4626
Conversation
leonardehrenfried
added
the
!New Feature
15ffac9 to
cf486ad
Compare
cf486ad to
0a78303
Compare
vesameskanen
left a comment
vesameskanen
left a comment
There was a problem hiding this comment.
I wonder if RemoveBikeParkWithShortBikeFilter.java should be renamed. The filter works well in cases where you take the bike into a vehicle instead of parking it. It does not make sense to drag the bike around without using it.
|
@vesameskanen while there is nothing parking-specific in the filter class itself, it's only ativated when the mode is BIKE_PARK. Are you interested in making it work for bike + transit searches, too? If so, should the same limits apply? |
|
Yes, I was wondering if code could be easily reused for other purposes in addition to BIKE_PARK street mode. I guess that might involve more parameters, as the purpose should be configurable. Minimum distance could be pretty much the same, but maybe the filter could be applied twice with different parameters. On side note: this filter, when applied alone, has an unpleasant side effect of removing best bike park suggestion and leaving some silly ones available. I got an itinerary with cycling to a park site away from the target station and much longer walk instead of the simple filtered one. Is there a way to prevent that, maybe using another filter before the new minimum distance filter? Perhaps the order of filtering actions is very significant here? |
|
Forget the side note, I just picked a bad start point which got mapped to a longer cycling route. Sorry. |
It should not be too difficult to implement this. We could reuse the same parameter or have two: one for BIKE_PARK and one for bike + transit. Do you have a suggestion how it should work? |
Codecov ReportBase: 60.61% // Head: 60.67% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #4626 +/- ##
=============================================
+ Coverage 60.61% 60.67% +0.05%
- Complexity 12212 12255 +43
=============================================
Files 1581 1581
Lines 63417 63514 +97
Branches 6992 6999 +7
=============================================
+ Hits 38438 38534 +96
- Misses 22800 22802 +2
+ Partials 2179 2178 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I am - very - sorry for slow feedback. The new approach looks good to me, but could the actual filter be parametrized with a given street mode? I am afraid that there will surely be a need for filtering 100 meter car park ride legs, super short scooter rentals etc. public RemoveItinerariesWithShortStreetLeg(double minDistance, TraverseMode mode) ... |
|
@vesameskanen Would you like the traverse mode also to be configurable through JSON or do you want to add that when you need it? |
|
I guess filtering is usually quite static need so parsing the values from json might be useful. New JSON params do not have to be implemented within the scope of this PR , though. New config parameters which activate street mode specific filtering can be added later. |
1e75562 to
84842e9
Compare
I've implemented this in 84842e9 |
| /** | ||
| * Remove all itineraries where all the final leg is more than x minutes of walking. | ||
| */ | ||
| public class FlexOnlyToDestinationFilter implements ItineraryListFilter { |
There was a problem hiding this comment.
Would it make sense to implement this similar to how maxTransferDuration is used, that way we could do the filtering already before the search?
There was a problem hiding this comment.
That's actually a very good point which I had not thought about.
leonardehrenfried
changed the title
| } | ||
|
|
||
| public TestItineraryBuilder flex(int start, int end, Place to) { | ||
| if (lastPlace == null) { |
There was a problem hiding this comment.
I'm going to leave this code in here even though it's not used right now. Took me a while to figure everything out and it will become useful at some stage.
a967a40 to
b1b486f
Compare
| var modes = request.journey().modes(); | ||
| boolean hasBikePark = List | ||
| .of(modes.accessMode, modes.egressMode) | ||
| .contains(StreetMode.BIKE_TO_PARK); | ||
|
|
||
| double minBikeParkingDistance = NOT_SET; | ||
| if (hasBikePark) { | ||
| minBikeParkingDistance = request.preferences().itineraryFilter().minBikeParkingDistance(); | ||
| } |
There was a problem hiding this comment.
Can we extract this, not to make the already complex method any mor complex. I'm also not really happy about using org.opentripplanner.routing.algorithm.filterchain.ItineraryListFilterChainBuilder.NOT_SET from here, can we find a better solution?
There was a problem hiding this comment.
Could we simply check if the limit is greater than zero?
There was a problem hiding this comment.
That sounds like a good idea
…n/ItineraryListFilterChainBuilder.java Co-authored-by: Hannes Junnila <hannes.junnila@gmail.com>
5 participants
Summary
This PR introduces two new filters for the Stadtnavi project's use cases:
- Flex only to destination: this is useful if you advertise a flex service that "brings you home". There you want to remove any itinerary that has a long-ish walking leg at the end.Unit tests
Lots added.
Documentation
Autogenerated.
cc @hbruch