Add generalizedCostMaxLimit field to the PageCursor to enable using RemoveTransitIfStreetOnlyIsBetter filter with paging
#6474
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| package org.opentripplanner.model.plan.paging.cursor; | ||
|
|
||
| import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResults; | ||
| import org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetterResults; | ||
| import org.opentripplanner.utils.tostring.ToStringBuilder; | ||
|
|
||
| /** | ||
| * This class stores input for the PageCursor. The input is related to the NumItinerariesFilter and RemoveTransitIfStreetOnlyIsBetter. | ||
| * <p> | ||
| * The NumItinerariesFilter removes itineraries from a list of itineraries based on the number to | ||
| * keep and whether it should crop at the head or the tail of the list. This class keeps | ||
| * the extreme endpoints of the sets of itineraries that were kept and removed, as well as more | ||
| * details about the first itinerary removed (bottom of the head, or top of the tail) and whether | ||
| * itineraries were cropped at the head or the tail. | ||
| * <p> | ||
| * The {@link org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetter} | ||
| * filter removes transit itineraries if the best street only itinerary has a lower cost. | ||
| * This class stores the cost of the best street only itinerary for use with paging. | ||
| */ | ||
| public class DefaultPageCursorInput implements PageCursorInput { | ||
|
|
||
| private final NumItinerariesFilterResults numItinerariesFilterResults; | ||
| private final RemoveTransitIfStreetOnlyIsBetterResults removeTransitIfStreetOnlyIsBetterResults; | ||
|
|
||
| private DefaultPageCursorInput() { | ||
| this.numItinerariesFilterResults = null; | ||
| this.removeTransitIfStreetOnlyIsBetterResults = null; | ||
| } | ||
|
|
||
| private DefaultPageCursorInput(Builder builder) { | ||
| this.numItinerariesFilterResults = builder.numItinerariesFilterResults(); | ||
| this.removeTransitIfStreetOnlyIsBetterResults = | ||
| builder.removeTransitIfStreetOnlyIsBetterResults(); | ||
| } | ||
|
|
||
| public static DefaultPageCursorInput.Builder of() { | ||
| return new Builder(new DefaultPageCursorInput()); | ||
| } | ||
|
|
||
| public DefaultPageCursorInput.Builder copyOf() { | ||
| return new Builder(this); | ||
| } | ||
|
|
||
| @Override | ||
| public NumItinerariesFilterResults numItinerariesFilterResults() { | ||
| return numItinerariesFilterResults; | ||
| } | ||
|
|
||
| @Override | ||
| public RemoveTransitIfStreetOnlyIsBetterResults removeTransitIfStreetOnlyIsBetterResults() { | ||
| return removeTransitIfStreetOnlyIsBetterResults; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return ToStringBuilder.of(DefaultPageCursorInput.class) | ||
| .addObj("numItinerariesFilterResults", numItinerariesFilterResults) | ||
| .addObj("removeTransitIfStreetOnlyIsBetterResults", removeTransitIfStreetOnlyIsBetterResults) | ||
| .toString(); | ||
| } | ||
|
|
||
| public static class Builder { | ||
|
|
||
| private NumItinerariesFilterResults numItinerariesFilterResults; | ||
| private RemoveTransitIfStreetOnlyIsBetterResults removeTransitIfStreetOnlyIsBetterResults; | ||
|
|
||
| public Builder(DefaultPageCursorInput original) { | ||
| this.numItinerariesFilterResults = original.numItinerariesFilterResults; | ||
| this.removeTransitIfStreetOnlyIsBetterResults = | ||
| original.removeTransitIfStreetOnlyIsBetterResults; | ||
| } | ||
|
|
||
| public NumItinerariesFilterResults numItinerariesFilterResults() { | ||
| return numItinerariesFilterResults; | ||
| } | ||
|
|
||
| public Builder withNumItinerariesFilterResults( | ||
| NumItinerariesFilterResults numItinerariesFilterResults | ||
| ) { | ||
| this.numItinerariesFilterResults = numItinerariesFilterResults; | ||
| return this; | ||
| } | ||
|
|
||
| public RemoveTransitIfStreetOnlyIsBetterResults removeTransitIfStreetOnlyIsBetterResults() { | ||
| return removeTransitIfStreetOnlyIsBetterResults; | ||
| } | ||
|
|
||
| public Builder withRemoveTransitIfStreetOnlyIsBetterResults( | ||
| RemoveTransitIfStreetOnlyIsBetterResults removeTransitIfStreetOnlyIsBetterResults | ||
| ) { | ||
| this.removeTransitIfStreetOnlyIsBetterResults = removeTransitIfStreetOnlyIsBetterResults; | ||
| return this; | ||
| } | ||
|
|
||
| public DefaultPageCursorInput build() { | ||
| return new DefaultPageCursorInput(this); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,25 @@ | ||
| package org.opentripplanner.model.plan.paging.cursor; | ||
|
|
||
| import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResults; | ||
| import org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetterResults; | ||
|
|
||
| /** | ||
| * This class holds information needed to create the next/previous page cursors either when there were | ||
| * itineraries removed due to cropping the list of itineraries using the numItineraries parameter or | ||
| * when the {@link org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetter} | ||
optionsome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * filter is used. | ||
| */ | ||
| public interface PageCursorInput { | ||
| /** | ||
| * This contains the results from {@link org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilter}. | ||
|
There was a problem hiding this comment. I could not find any already used imports that were relevant here. ...Filter vs ...FilterResults There was a problem hiding this comment. FYI: The There was a problem hiding this comment. The There was a problem hiding this comment. Did we decide what we should do about this? There was a problem hiding this comment. We still haven't decided what to do about this? |
||
| * The Instant fields in NumItinerariesFilterResults come from the sets of itineraries that were removed and the ones that were | ||
| * kept as a result of using the numItineraries parameter. | ||
|
There was a problem hiding this comment. This sounds like documentation that should be in the NumItinerariesFilterResults instead of here. There was a problem hiding this comment. This is documented in NumItinerariesFilterResults already so I just removed it There was a problem hiding this comment. There is a lot of do referencing the The itinerary-filter-chain may crop the search-window as a result of reducing the list down to The new feature crop the transit result based on generalized-cost (search-window crop on time). Futher the cropping of the search-window is just a slice(page-cut) between the current result and the next, while the There was a problem hiding this comment. I changed the comments slightly here |
||
| */ | ||
| NumItinerariesFilterResults numItinerariesFilterResults(); | ||
optionsome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /** | ||
| * RemoveTransitIfStreetOnlyIsBetterResults contains the best street only cost that comes from taking the cost of the best street only itinerary from the first search. | ||
| * This is used as a comparison in {@link org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetter} | ||
| * when paging is used. | ||
| */ | ||
| RemoveTransitIfStreetOnlyIsBetterResults removeTransitIfStreetOnlyIsBetterResults(); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely understand these changes. In what scenario is
numItinerariesFilterResultsnull and why iswholeSwUsedset as false only ifnumItinerariesFilterResultsis not null?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numItinerariesFilterResults() can be null if no itineraries were removed, I added a comment
OpenTripPlanner/application/src/main/java/org/opentripplanner/routing/algorithm/filterchain/filters/system/NumItinerariesFilter.java
Line 46 in c1626ff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above comment also requires looking at the whole context of the filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may rename
wholeSwUsedtowholeSearchWindowUsed, but it it is the case that the state of thewholeSearchWindowUsedcan be derived fromis pageCursorInput.numItinerariesFilterResults() == null, replacing it with a method is maybe better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed
wholeSwUsedtowholeSearchWindowUsed, but did not create a methodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also by default true. Usually we try to keep false as the default value. Also, the toString method uses a different name for this, perhaps we should use that for either the variable name or as the method name as @t2gran suggested.