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

Create the NumItinerariesFilter and the associated NumItinerariesFilterResults to limit itineraries based on the numItineraries parameter. #5450

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

eibakke
Copy link
Contributor

@eibakke eibakke commented Oct 23, 2023

Summary

This PR pulls functionality to limit the number of itineraries to the numItineraries parameter into a separate filter from the MaxLimitFilter because of the need to keep more information about the filtered itineraries than just the first one that is removed. The NumItinerariesFilterResults contains information used in the creation of page cursors currently, as well as information that will be used in the future deduplication feature.

Issue

N/A

Unit tests

Updated relevant unit tests and added a unittest for the new filter.

Documentation

Added JavaDoc.

Changelog

N/A

Bumping the serialization version id

N/A

…terResults to limit itineraries based on the numItineraries parameter.

Pulling this functionality into a separate filter from the MaxLimitFilter because of the need to keep more information about the filtered itineraries than just the first one that is removed. The NumItinerariesFilterResults contains information used in the page cursors currently as well as information that will be used in the future deduplication feature.
@eibakke eibakke requested a review from a team as a code owner October 23, 2023 12:56
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (c608267) 66.79% compared to head (c0d42a2) 66.80%.
Report is 97 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5450      +/-   ##
=============================================
+ Coverage      66.79%   66.80%   +0.01%     
- Complexity     15463    15556      +93     
=============================================
  Files           1798     1806       +8     
  Lines          69795    69821      +26     
  Branches        7357     7357              
=============================================
+ Hits           46619    46644      +25     
+ Misses         20720    20719       -1     
- Partials        2456     2458       +2     
Files Coverage Δ
...er/ext/flex/template/FlexAccessEgressTemplate.java 71.42% <ø> (ø)
.../main/java/org/opentripplanner/model/StopTime.java 77.27% <ø> (ø)
...java/org/opentripplanner/model/plan/SortOrder.java 100.00% <ø> (ø)
...anner/model/plan/pagecursor/PageCursorFactory.java 98.52% <100.00%> (-0.16%) ⬇️
...entripplanner/routing/algorithm/RoutingWorker.java 80.70% <100.00%> (ø)
...m/filterchain/ItineraryListFilterChainBuilder.java 90.00% <100.00%> (ø)
...ner/routing/algorithm/filterchain/ListSection.java 100.00% <ø> (ø)
...hm/filterchain/deletionflagger/MaxLimitFilter.java 100.00% <ø> (ø)
...terchain/deletionflagger/NumItinerariesFilter.java 100.00% <100.00%> (ø)
...n/deletionflagger/NumItinerariesFilterResults.java 100.00% <100.00%> (ø)
... and 6 more

... and 60 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I must admit that I don't fully understand the logic here so I would appreciate some context in a dev meeting.

Besides I picked up on a small thing.

@eibakke
Copy link
Contributor Author

eibakke commented Oct 26, 2023

I must admit that I don't fully understand the logic here so I would appreciate some context in a dev meeting.

The idea is to capture more information about the result of applying the numItineraries condition and using it in the pageCursor for more accurate paging. I'll use it next in order to get rid of potential duplicates when paging. I'll happily explain more in today's meeting.

Besides I picked up on a small thing.

Taking a look at the timestamp addition suggestions and the question you had now.

…terResults to limit itineraries based on the numItineraries parameter.

Pulling this functionality into a separate filter from the MaxLimitFilter because of the need to keep more information about the filtered itineraries than just the first one that is removed. The NumItinerariesFilterResults contains information used in the page cursors currently as well as information that will be used in the future deduplication feature.
…terResults to limit itineraries based on the numItineraries parameter.

Pulling this functionality into a separate filter from the MaxLimitFilter because of the need to keep more information about the filtered itineraries than just the first one that is removed. The NumItinerariesFilterResults contains information used in the page cursors currently as well as information that will be used in the future deduplication feature.
vpaturet
vpaturet previously approved these changes Oct 27, 2023
Copy link
Contributor

@vpaturet vpaturet left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@t2gran t2gran merged commit b0f876e into opentripplanner:dev-2.x Nov 1, 2023
5 checks passed
@t2gran t2gran deleted the num_itin_filter branch November 1, 2023 13:50
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.

4 participants