Skip to content

Allow filtering out of timeshifted itineraries with same routes and stops - #4298

Merged
leonardehrenfried merged 14 commits into
dev-2.xfrom
unknown repository
Aug 8, 2022
Merged

Allow filtering out of timeshifted itineraries with same routes and stops#4298
leonardehrenfried merged 14 commits into
dev-2.xfrom
unknown repository

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Jul 25, 2022

Copy link
Copy Markdown
Member

Summary

Adds a filter that removes itineraries from the result set if they are found to be using the same stops/stations and routes but at different times. This is useful if you don't care so much about "best" results but want to show a greater variety.

I'm a little unsure about the name of the config param: removeTimeShiftedDuplicates. Does anyone have a better idea?

Configuration example

router-config.json

{
  "routingDefaults": {
    "itineraryFilters" : {
      "removeItinerariesWithSameRoutesAndStops" : true
    }
  }
}

Issue

Closes #4274

Unit tests

Unit and module tests added.

Documentation

Added.

@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner July 25, 2022 09:24
@leonardehrenfried leonardehrenfried added the !Improvement A functional improvement or micro feature label Jul 25, 2022
@leonardehrenfried

Copy link
Copy Markdown
Member Author

cc @ivanwimt @FrancoisG-WIMT

@codecov-commenter

codecov-commenter commented Jul 25, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4298 (c95e835) into dev-2.x (f1b43ce) will increase coverage by 0.32%.
The diff coverage is 94.28%.

@@              Coverage Diff              @@
##             dev-2.x    #4298      +/-   ##
=============================================
+ Coverage      57.23%   57.55%   +0.32%     
- Complexity     10792    10941     +149     
=============================================
  Files           1429     1442      +13     
  Lines          58013    58285     +272     
  Branches        6729     6773      +44     
=============================================
+ Hits           33202    33548     +346     
+ Misses         22799    22714      -85     
- Partials        2012     2023      +11     
Impacted Files Coverage Δ
...nner/standalone/config/ItineraryFiltersMapper.java 11.11% <0.00%> (-0.43%) ⬇️
...m/filterchain/ItineraryListFilterChainBuilder.java 91.50% <100.00%> (+0.69%) ⬆️
...m/filterchain/groupids/GroupByAllSameStations.java 73.33% <100.00%> (-4.45%) ⬇️
...ilterchain/groupids/GroupBySameRoutesAndStops.java 100.00% <100.00%> (ø)
...thm/mapping/RoutingRequestToFilterChainMapper.java 77.77% <100.00%> (+0.85%) ⬆️
...routing/api/request/ItineraryFilterParameters.java 100.00% <100.00%> (ø)
...entripplanner/transit/model/site/StopLocation.java 62.50% <100.00%> (+8.65%) ⬆️
...ain/java/org/opentripplanner/model/StreetNote.java 56.25% <0.00%> (-43.75%) ⬇️
...r/routing/services/notes/MatcherAndStreetNote.java 63.63% <0.00%> (-36.37%) ⬇️
...va/org/opentripplanner/ext/flex/trip/FlexTrip.java 71.42% <0.00%> (-11.91%) ⬇️
... and 129 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hannesj
hannesj previously approved these changes Jul 25, 2022
@t2gran t2gran added this to the 2.2 milestone Jul 26, 2022
@t2gran
t2gran self-requested a review July 26, 2022 08:31
@leonardehrenfried

Copy link
Copy Markdown
Member Author

At today's meeting we decided that we don't want to overload the term "duplicate" and will use a very wordy, but descriptive config param: removeTimeshiftedItinerariesWithSameRoutesAndStops.

@leonardehrenfried leonardehrenfried changed the title Allow filtering out of timeshifted "duplicates" Allow filtering out of timeshifted itineraries with same routes and stops Jul 26, 2022
hannesj
hannesj previously approved these changes Jul 27, 2022
hannesj
hannesj previously approved these changes Jul 28, 2022
Comment thread docs/RouterConfiguration.md Outdated
new GroupByFilter<>(
GroupBySameRoutesAndStops::new,
List.of(
new SortingFilter(generalizedCostComparator()),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to keep the lowest cost or the earliest departure?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep the earliest departure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be the earliest itinerary for a depart-after search, and the latest for a arrive-by search?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give me a hint how to achieve that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I be passing the arriveBy flag through to the chain and then the the ListSection to head or tail respectively?

@t2gran t2gran Aug 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this this would work: new GroupByFilter<>(, new SortingFilter(SortOrderComparator.comparator(sortOrder)) .... Strictly, this will sort street itineraries by them selves - but I guess we do not care in this case, so it is ok.

Can you move the new GroupByFilter<>(...) into a builder method as well, like the buildGroupByTripIdAndDistanceFilters()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for both.

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 still sorting on generalized cost.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, now it's really fixed.

hannesj
hannesj previously approved these changes Aug 8, 2022
hannesj
hannesj previously approved these changes Aug 8, 2022
@leonardehrenfried
leonardehrenfried merged commit e3f0f2b into opentripplanner:dev-2.x Aug 8, 2022
t2gran pushed a commit that referenced this pull request Aug 8, 2022
@leonardehrenfried
leonardehrenfried deleted the timeshifted-duplicates branch August 8, 2022 14:26
@miklcct

miklcct commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

Is it possible to expose this parameter in the GraphQL APIs?

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

Labels

!Improvement A functional improvement or micro feature WhereIsMyTransport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add filter to remove timeshifted "identical" itineraries

5 participants