Skip to content

Use TripPatterns for filtering - #4820

Merged
leonardehrenfried merged 12 commits into
opentripplanner:dev-2.xfrom
entur:otp2_subode_filter
Feb 16, 2023
Merged

Use TripPatterns for filtering#4820
leonardehrenfried merged 12 commits into
opentripplanner:dev-2.xfrom
entur:otp2_subode_filter

Conversation

@hannesj

@hannesj hannesj commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

Summary

There is a couple of issues with the current filtering code:

  • Unable to include trips on patterns, where the trip does not match the route mode or submode.
  • matchTripTimes did not use the not filter.

This fixes these issues, by doing the filtering on the pattern level instead, and adding a boolean flag on the pattern, containing information whether the pattern contains multiple modes or submodes.

Also some minor performance optimizations were done.

@hannesj hannesj added !Bug Apply to issues describing a bug and PRs witch fixes it. Entur On Entur Roadmap !Optimization The feature is to improve performance. labels Feb 9, 2023
@hannesj hannesj added this to the 2.3 milestone Feb 9, 2023
@hannesj
hannesj requested a review from a team as a code owner February 9, 2023 10:15
@codecov

codecov Bot commented Feb 9, 2023

Copy link
Copy Markdown

Codecov Report

Base: 62.14% // Head: 62.12% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (f9edd71) compared to base (b0b5f36).
Patch coverage: 62.35% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #4820      +/-   ##
=============================================
- Coverage      62.14%   62.12%   -0.02%     
- Complexity     13000    13005       +5     
=============================================
  Files           1650     1650              
  Lines          66064    66105      +41     
  Branches        7184     7198      +14     
=============================================
+ Hits           41055    41070      +15     
- Misses         22669    22688      +19     
- Partials        2340     2347       +7     
Impacted Files Coverage Δ
...pplanner/ext/siri/SiriTimetableSnapshotSource.java 0.00% <0.00%> (ø)
...opentripplanner/ext/siri/SiriTripPatternCache.java 0.00% <0.00%> (ø)
...lanner/ext/transmodelapi/mapping/FilterMapper.java 0.00% <0.00%> (ø)
...tripplanner/ext/traveltime/TravelTimeResource.java 0.00% <ø> (ø)
...ostopheuristics/VehicleToStopSkipEdgeStrategy.java 0.00% <0.00%> (ø)
...planner/graph_builder/module/NearbyStopFinder.java 85.58% <0.00%> (ø)
...ripplanner/model/modes/AllowTransitModeFilter.java 100.00% <ø> (ø)
...ipplanner/model/modes/ExcludeAllTransitFilter.java 50.00% <ø> (ø)
.../algorithm/raptoradapter/router/TransitRouter.java 75.67% <ø> (-0.22%) ⬇️
.../request/request/filter/AllowAllTransitFilter.java 100.00% <ø> (ø)
... and 18 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leonardehrenfried

leonardehrenfried commented Feb 9, 2023

Copy link
Copy Markdown
Member

I'm currently testing another fix for the Windows problem: 004eeb4

@hannesj hannesj mentioned this pull request Feb 9, 2023

@t2gran t2gran left a comment

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.

Looked fast through this and it looks ok.

@hannesj hannesj added +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR Entur Test This is currently being tested at Entur labels Feb 10, 2023
@hannesj
hannesj requested a review from t2gran February 14, 2023 10:16
t2gran
t2gran previously approved these changes Feb 14, 2023
// then we do not have to do that at all and it should increase performance a lot
// trip has to match with at least one predicate in order to be included in search
// Trip has to match with at least one predicate in order to be included in search. We only have
// to this if we have mode specific filters, and not all trips on hte pattern have the same

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.

the*


public List<SelectRequest> not() {
return not;
return Collections.unmodifiableList(Arrays.asList(select));

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.

I think this should be not

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, we no longer expose not, as it did have no usage.

@leonardehrenfried

leonardehrenfried commented Feb 16, 2023

Copy link
Copy Markdown
Member

To unblock @Bartosz-Kruba I'm going to merge this.

@leonardehrenfried
leonardehrenfried merged commit f8afbd4 into opentripplanner:dev-2.x Feb 16, 2023
t2gran pushed a commit that referenced this pull request Feb 16, 2023
t2gran pushed a commit that referenced this pull request Feb 16, 2023
@hannesj
hannesj deleted the otp2_subode_filter branch February 16, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR Entur Test This is currently being tested at Entur Entur On Entur Roadmap !Optimization The feature is to improve performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants