Skip to content

Add generalizedCostMaxLimit field to the PageCursor to enable using RemoveTransitIfStreetOnlyIsBetter filter with paging #6474

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

Merged
merged 59 commits into from
May 22, 2025

Conversation

VillePihlava
Copy link
Contributor

@VillePihlava VillePihlava commented Feb 20, 2025

Summary

This PR adds the generalizedCostMaxLimit field to the PageCursor. This enables using the RemoveTransitIfStreetOnlyIsBetter filter with paging. Before this PR, this was not possible because paging only used the walk mode to create a direct itinerary which made the filter work incorrectly.

Issue

N/A

Unit tests

Changed existing unit tests for compatibility and created new ones. Also performed manual testing.

Documentation

Code comments, no other documentation

@VillePihlava VillePihlava requested a review from a team as a code owner February 20, 2025 12:44
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 83.64780% with 26 lines in your changes missing coverage. Please review.

Project coverage is 71.26%. Comparing base (9a5c2aa) to head (ffbc2fd).
Report is 769 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...thm/filterchain/paging/DefaultPageCursorInput.java 83.72% 7 Missing ⚠️
...ain/filters/system/NumItinerariesFilterResult.java 0.00% 6 Missing ⚠️
.../opentripplanner/service/paging/PagingService.java 66.66% 1 Missing and 3 partials ⚠️
...ansit/RemoveTransitIfStreetOnlyIsBetterResult.java 25.00% 3 Missing ⚠️
...m/filterchain/ItineraryListFilterChainBuilder.java 81.81% 2 Missing ⚠️
...rithm/mapping/RouteRequestToFilterChainMapper.java 33.33% 1 Missing and 1 partial ⚠️
...er/model/plan/paging/cursor/PageCursorFactory.java 87.50% 1 Missing ⚠️
...entripplanner/routing/algorithm/RoutingWorker.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6474      +/-   ##
=============================================
+ Coverage      70.25%   71.26%   +1.01%     
- Complexity     18385    18486     +101     
=============================================
  Files           2088     2031      -57     
  Lines          77417    76396    -1021     
  Branches        7840     7807      -33     
=============================================
+ Hits           54386    54445      +59     
+ Misses         20262    19190    -1072     
+ Partials        2769     2761       -8     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@VillePihlava
Copy link
Contributor Author

I removed the direct mode search with paging and refactored bestStreetOnlyCost to streetOnlyCost. This should now be ready-for-review.

@VillePihlava VillePihlava changed the title Add bestStreetOnlyCost field to the PageCursor to enable using RemoveTransitIfStreetOnlyIsBetter filter with paging Add streetOnlyCost field to the PageCursor to enable using RemoveTransitIfStreetOnlyIsBetter filter with paging Feb 21, 2025
@VillePihlava VillePihlava changed the title Add streetOnlyCost field to the PageCursor to enable using RemoveTransitIfStreetOnlyIsBetter filter with paging Add generalizedCostMaxLimit field to the PageCursor to enable using RemoveTransitIfStreetOnlyIsBetter filter with paging Mar 6, 2025
@t2gran t2gran added this to the 2.8 (next release) milestone Mar 12, 2025
@optionsome optionsome requested review from t2gran and optionsome April 8, 2025 09:15
Comment on lines +23 to 28

/**
* The latest removed departure together with a search window duration is used to
* calculate the start of the search window preceding the current window.
*/
Instant latestRemovedDeparture();
Copy link
Member

Choose a reason for hiding this comment

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

Document whether this is the arrival or departure time of the latest removed departure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think because the name has departure in it, it is clear enough, is that ok? The word "arrival" (or an abbreviation) is usually included in variables that deal with arrival times, for example in PageCursorFactory the variable lat (latest arrival time)

Copy link
Member

Choose a reason for hiding this comment

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

latestRemovedDepartureTime would be slightly more explicit as departure is sometimes a synonym to trip and a trip can be identified by an arrival time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there was a comment here from Thomas that agreed with me, but has now disappeared. Is keeping the name the same ok for you @optionsome

@VillePihlava VillePihlava requested a review from optionsome April 29, 2025 07:04
@optionsome optionsome removed the request for review from eibakke April 29, 2025 08:27
Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

This still needs structural changes - I will complete my review when these are done. I have not yet looked at the details.

import org.opentripplanner.model.plan.ItinerarySortKey;
import org.opentripplanner.model.plan.SortOrder;
import org.opentripplanner.routing.algorithm.filterchain.paging.DefaultPageCursorInput;
Copy link
Member

Choose a reason for hiding this comment

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

This still have a dependency to filterchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this and modified logic related to null checking in the PageCursorFactory and PagingService. I noticed that the assertRequestPrerequisites function in PagingService is useless because of a preceding check. Should I remove it?

@VillePihlava VillePihlava requested a review from t2gran May 5, 2025 06:33
Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

This looks good now all the architecture comments I had is resolved. There is just a few minor things. When these are fixed I can approve.

@VillePihlava VillePihlava requested a review from t2gran May 13, 2025 14:00
@VillePihlava VillePihlava merged commit e7ec87f into opentripplanner:dev-2.x May 22, 2025
7 checks passed
t2gran pushed a commit that referenced this pull request May 22, 2025
@VillePihlava VillePihlava deleted the paging-filtering-fix branch May 22, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement A functional improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants