Remove transit with higher cost than best on-street itinerary filter

docs/RouterConfiguration.md

@@ -504,6 +504,8 @@ HTTP headers to add to the request. Any header key, value can be inserted.
         "costLimitFunction" : "900 + 1.5 x",
         "intervalRelaxFactor" : 0.4
       },
+      "nonTransitGeneralizedCostLimit" : "400 + 1.5x",
+      "removeTransitWithHigherCostThanBestOnStreetOnly" : "60 + 1.3x",
       "bikeRentalDistanceRatio" : 0.3,
       "accessibilityScore" : true,
       "minBikeParkingDistance" : 300,

src/main/java/org/opentripplanner/routing/algorithm/filterchain/ItineraryListFilterChainBuilder.java

@@ -23,6 +23,7 @@
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveItinerariesWithShortStreetLeg;
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveParkAndRideWithMostlyWalkingFilter;
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveTransitIfStreetOnlyIsBetterFilter;
+import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveTransitIfWalkingIsBetterFilter;
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveWalkOnlyFilter;
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.TransitGeneralizedCostFilter;
 import org.opentripplanner.routing.algorithm.filterchain.filter.DeletionFlaggingFilter;
@@ -56,7 +57,7 @@ public class ItineraryListFilterChainBuilder {
   private ItineraryFilterDebugProfile debug = ItineraryFilterDebugProfile.OFF;
   private int maxNumberOfItineraries = NOT_SET;
   private ListSection maxNumberOfItinerariesCrop = ListSection.TAIL;
-  private boolean removeTransitWithHigherCostThanBestOnStreetOnly = true;
+  private CostLinearFunction removeTransitWithHigherCostThanBestOnStreetOnly;
   private boolean removeWalkAllTheWayResults;
   private boolean sameFirstOrLastTripFilter;
   private TransitGeneralizedCostFilterParams transitGeneralizedCostFilterParams;
@@ -72,6 +73,7 @@ public class ItineraryListFilterChainBuilder {
   private Function<Station, MultiModalStation> getMultiModalStation;
   private boolean removeItinerariesWithSameRoutesAndStops;
   private double minBikeParkingDistance;
+  private boolean removeTransitIfWalkingIsBetter = true;
 
   @Sandbox
   private ItineraryListFilter rideHailingFilter;
@@ -184,7 +186,7 @@ public ItineraryListFilterChainBuilder withParkAndRideDurationRatio(double value
   /**
    * The direct street search(walk, bicycle, car) is not pruning the transit search, so in some
    * cases we get "silly" transit itineraries that is marginally better on travel-duration compared
-   * with a on-street-all-the-way itinerary. Use this method to turn this filter on/off.
+   * with a on-street-all-the-way itinerary. Use this method to turn filter worse enough itineraries.
    * <p>
    * The filter remove all itineraries with a generalized-cost that is higher than the best
    * on-street-all-the-way itinerary.
@@ -193,12 +195,23 @@ public ItineraryListFilterChainBuilder withParkAndRideDurationRatio(double value
    * exist.
    */
   public ItineraryListFilterChainBuilder withRemoveTransitWithHigherCostThanBestOnStreetOnly(
-    boolean value
+    CostLinearFunction value
   ) {
     this.removeTransitWithHigherCostThanBestOnStreetOnly = value;
     return this;
   }
 
+  /**
+   * An itinerary which has transit legs and more walking than the plain walk itinerary are silly.
+   * This filter removes such itineraries.
+   * <p>
+   * This filter only have an effect, if an walk-all-the-way itinerary exist.
+   */
+  public ItineraryListFilterChainBuilder withRemoveTransitIfWalkingIsBetter(boolean value) {
+    this.removeTransitIfWalkingIsBetter = value;
+    return this;
+  }
+
   /**
    * This will NOT delete itineraries, but tag them as deleted using the {@link
    * Itinerary#getSystemNotices()}.
@@ -350,8 +363,19 @@ public ItineraryListFilterChain build() {
     // is worse). B is removed by the {@link LatestDepartureTimeFilter} below. This is exactly
     // what we want, since both itineraries are none optimal.
     {
-      if (removeTransitWithHigherCostThanBestOnStreetOnly) {
-        filters.add(new DeletionFlaggingFilter(new RemoveTransitIfStreetOnlyIsBetterFilter()));
+      // Filter transit itineraries by comparing against non-transit using generalized-cost
+      if (removeTransitWithHigherCostThanBestOnStreetOnly != null) {
+        filters.add(
+          new DeletionFlaggingFilter(
+            new RemoveTransitIfStreetOnlyIsBetterFilter(
+              removeTransitWithHigherCostThanBestOnStreetOnly
+            )
+          )
+        );
+      }
+
+      if (removeTransitIfWalkingIsBetter) {
+        filters.add(new DeletionFlaggingFilter(new RemoveTransitIfWalkingIsBetterFilter()));
       }
 
       if (removeWalkAllTheWayResults) {

src/main/java/org/opentripplanner/routing/algorithm/filterchain/RoutingErrorsAttacher.java

@@ -11,6 +11,7 @@
 import org.opentripplanner.model.plan.StreetLeg;
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.OutsideSearchWindowFilter;
 import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveTransitIfStreetOnlyIsBetterFilter;
+import org.opentripplanner.routing.algorithm.filterchain.deletionflagger.RemoveTransitIfWalkingIsBetterFilter;
 import org.opentripplanner.routing.api.response.RoutingError;
 
 /**
@@ -50,7 +51,16 @@ static List<RoutingError> computeErrors(
           .getSystemNotices()
           .stream()
           .anyMatch(notice -> notice.tag.equals(RemoveTransitIfStreetOnlyIsBetterFilter.TAG));
-      if (filteredItineraries.stream().allMatch(isOnStreetAllTheWay.or(isWorseThanStreet))) {
+      Predicate<Itinerary> isWorseThanWalking = it ->
+        it
+          .getSystemNotices()
+          .stream()
+          .anyMatch(notice -> notice.tag.equals(RemoveTransitIfWalkingIsBetterFilter.TAG));
+      if (
+        filteredItineraries
+          .stream()
+          .allMatch(isOnStreetAllTheWay.or(isWorseThanStreet).or(isWorseThanWalking))
+      ) {
         var nonTransitIsWalking = filteredItineraries
           .stream()
           .flatMap(Itinerary::getStreetLegs)

src/main/java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/RemoveTransitIfStreetOnlyIsBetterFilter.java

@@ -2,17 +2,27 @@
 
 import java.util.Comparator;
 import java.util.List;
-import java.util.Optional;
+import java.util.OptionalDouble;
+import java.util.OptionalInt;
 import java.util.stream.Collectors;
+import org.opentripplanner.framework.model.Cost;
 import org.opentripplanner.model.plan.Itinerary;
+import org.opentripplanner.model.plan.Leg;
+import org.opentripplanner.routing.api.request.framework.CostLinearFunction;
 
 /**
  * Filter itineraries based on generalizedCost, compared with a on-street-all-the-way itinerary(if
- * it exist). If an itinerary is slower than the best all-the-way-on-street itinerary, then the
+ * it exist). If an itinerary cost exceeds the limit computed from the best all-the-way-on-street itinerary, then the
  * transit itinerary is removed.
  */
 public class RemoveTransitIfStreetOnlyIsBetterFilter implements ItineraryDeletionFlagger {
 
+  private final CostLinearFunction costLimitFunction;
+
+  public RemoveTransitIfStreetOnlyIsBetterFilter(CostLinearFunction costLimitFunction) {
+    this.costLimitFunction = costLimitFunction;
+  }
+
   /**
    * Required for {@link org.opentripplanner.routing.algorithm.filterchain.ItineraryListFilterChain},
    * to know which filters removed
@@ -26,19 +36,22 @@ public String name() {
 
   @Override
   public List<Itinerary> flagForRemoval(List<Itinerary> itineraries) {
-    // Find the best walk-all-the-way option
-    Optional<Itinerary> bestStreetOp = itineraries
+    // Find the best street-all-the-way option
+    OptionalInt minStreetCost = itineraries
       .stream()
       .filter(Itinerary::isOnStreetAllTheWay)
-      .min(Comparator.comparingInt(Itinerary::getGeneralizedCost));
+      .mapToInt(Itinerary::getGeneralizedCost)
+      .min();
 
-    if (bestStreetOp.isEmpty()) {
+    if (minStreetCost.isEmpty()) {
       return List.of();
     }
 
-    final long limit = bestStreetOp.get().getGeneralizedCost();
+    var limit = costLimitFunction
+      .calculate(Cost.costOfSeconds(minStreetCost.getAsInt()))
+      .toSeconds();
 
-    // Filter away itineraries that have higher cost than the best non-transit option.
+    // Filter away itineraries that have higher cost than limit cost computed above
     return itineraries
       .stream()
       .filter(it -> !it.isOnStreetAllTheWay() && it.getGeneralizedCost() >= limit)

src/main/java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/RemoveTransitIfWalkingIsBetterFilter.java

@@ -0,0 +1,56 @@
+package org.opentripplanner.routing.algorithm.filterchain.deletionflagger;
+
+import java.util.Comparator;
+import java.util.List;
+import java.util.OptionalDouble;
+import java.util.OptionalInt;
+import java.util.stream.Collectors;
+import org.opentripplanner.framework.model.Cost;
+import org.opentripplanner.model.plan.Itinerary;
+import org.opentripplanner.model.plan.Leg;
+import org.opentripplanner.routing.api.request.framework.CostLinearFunction;
+
+/**
+ * Filter itineraries which contain more walking than a pure walk itinerary
+ */
+public class RemoveTransitIfWalkingIsBetterFilter implements ItineraryDeletionFlagger {
+
+  /**
+   * Required for {@link org.opentripplanner.routing.algorithm.filterchain.ItineraryListFilterChain},
+   * to know which filters removed
+   */
+  public static final String TAG = "transit-vs-walk-filter";
+
+  @Override
+  public String name() {
+    return TAG;
+  }
+
+  @Override
+  public List<Itinerary> flagForRemoval(List<Itinerary> itineraries) {
+    // Filter the most common silly itinerary case: transit itinerary has more walking than plain walk itinerary
+    // This never makes sense
+
+    OptionalInt minWalkCost = itineraries
+      .stream()
+      .filter(Itinerary::isWalkingAllTheWay)
+      .mapToInt(Itinerary::getGeneralizedCost)
+      .min();
+
+    if (minWalkCost.isEmpty()) {
+      return List.of();
+    }
+
+    var limit = minWalkCost.getAsInt();
+
+    return itineraries
+      .stream()
+      .filter(it -> !it.isOnStreetAllTheWay() && it.getGeneralizedCost() >= limit)
+      .collect(Collectors.toList());
+  }
+
+  @Override
+  public boolean skipAlreadyFlaggedItineraries() {
+    return false;
+  }
+}

src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java

@@ -65,6 +65,9 @@ public static ItineraryListFilterChain createFilterChain(
       .withBikeRentalDistanceRatio(params.bikeRentalDistanceRatio())
       .withParkAndRideDurationRatio(params.parkAndRideDurationRatio())
       .withNonTransitGeneralizedCostLimit(params.nonTransitGeneralizedCostLimit())
+      .withRemoveTransitWithHigherCostThanBestOnStreetOnly(
+        params.removeTransitWithHigherCostThanBestOnStreetOnly()
+      )
       .withSameFirstOrLastTripFilter(params.filterItinerariesWithSameFirstOrLastTrip())
       .withAccessibilityScore(
         params.useAccessibilityScore() && request.wheelchair(),
@@ -79,10 +82,10 @@ public static ItineraryListFilterChain createFilterChain(
         context.transitService().getTransitAlertService(),
         context.transitService()::getMultiModalStationForStation
       )
-      .withRemoveTransitWithHigherCostThanBestOnStreetOnly(true)
       .withLatestDepartureTimeLimit(filterOnLatestDepartureTime)
       .withMaxLimitReachedSubscriber(maxLimitReachedSubscriber)
       .withRemoveWalkAllTheWayResults(removeWalkAllTheWayResults)
+      .withRemoveTransitIfWalkingIsBetter(true)
       .withDebugEnabled(params.debug());
 
     if (!context.rideHailingServices().isEmpty()) {

src/main/java/org/opentripplanner/routing/api/request/preference/ItineraryFilterPreferences.java

@@ -30,6 +30,8 @@ public final class ItineraryFilterPreferences {
   private final double parkAndRideDurationRatio;
   private final boolean removeItinerariesWithSameRoutesAndStops;
   private final TransitGeneralizedCostFilterParams transitGeneralizedCostLimit;
+  private final CostLinearFunction removeTransitWithHigherCostThanBestOnStreetOnly;
+  private final boolean removeTransitWithMoreWalking;
 
   private ItineraryFilterPreferences() {
     this.accessibilityScore = false;
@@ -48,6 +50,9 @@ private ItineraryFilterPreferences() {
         CostLinearFunction.of(Duration.ofMinutes(15), 1.5),
         0.4
       );
+    this.removeTransitWithHigherCostThanBestOnStreetOnly =
+      CostLinearFunction.of(Duration.ofMinutes(1), 1.3);
+    this.removeTransitWithMoreWalking = false;
   }
 
   private ItineraryFilterPreferences(Builder builder) {
@@ -66,6 +71,9 @@ private ItineraryFilterPreferences(Builder builder) {
     this.parkAndRideDurationRatio = Units.ratio(builder.parkAndRideDurationRatio);
     this.removeItinerariesWithSameRoutesAndStops = builder.removeItinerariesWithSameRoutesAndStops;
     this.transitGeneralizedCostLimit = Objects.requireNonNull(builder.transitGeneralizedCostLimit);
+    this.removeTransitWithHigherCostThanBestOnStreetOnly =
+      Objects.requireNonNull(builder.removeTransitWithHigherCostThanBestOnStreetOnly);
+    this.removeTransitWithMoreWalking = builder.removeTransitWithMoreWalking;
   }
 
   public static Builder of() {
@@ -124,6 +132,14 @@ public TransitGeneralizedCostFilterParams transitGeneralizedCostLimit() {
     return transitGeneralizedCostLimit;
   }
 
+  public CostLinearFunction removeTransitWithHigherCostThanBestOnStreetOnly() {
+    return removeTransitWithHigherCostThanBestOnStreetOnly;
+  }
+
+  public boolean removeTransitWithMoreWalking() {
+    return removeTransitWithMoreWalking;
+  }
+
   @Override
   public String toString() {
     return ToStringBuilder
@@ -162,10 +178,16 @@ public String toString() {
         transitGeneralizedCostLimit,
         DEFAULT.transitGeneralizedCostLimit
       )
+      .addObj(
+        "removeTransitWithHigherCostThanBestOnStreetOnly",
+        removeTransitWithHigherCostThanBestOnStreetOnly,
+        DEFAULT.removeTransitWithHigherCostThanBestOnStreetOnly
+      )
       .addBoolIfTrue(
         "removeItinerariesWithSameRoutesAndStops",
         removeItinerariesWithSameRoutesAndStops
       )
+      .addBoolIfTrue("removeTransitWithMoreWalking", removeTransitWithMoreWalking)
       .toString();
   }
 
@@ -178,6 +200,7 @@ public boolean equals(Object o) {
       accessibilityScore == that.accessibilityScore &&
       Double.compare(that.bikeRentalDistanceRatio, bikeRentalDistanceRatio) == 0 &&
       debug == that.debug &&
+      removeTransitWithMoreWalking == that.removeTransitWithMoreWalking &&
       filterItinerariesWithSameFirstOrLastTrip == that.filterItinerariesWithSameFirstOrLastTrip &&
       Double.compare(
         that.groupedOtherThanSameLegsMaxCostMultiplier,
@@ -190,6 +213,10 @@ public boolean equals(Object o) {
       Double.compare(that.parkAndRideDurationRatio, parkAndRideDurationRatio) == 0 &&
       removeItinerariesWithSameRoutesAndStops == that.removeItinerariesWithSameRoutesAndStops &&
       Objects.equals(nonTransitGeneralizedCostLimit, that.nonTransitGeneralizedCostLimit) &&
+      Objects.equals(
+        removeTransitWithHigherCostThanBestOnStreetOnly,
+        that.removeTransitWithHigherCostThanBestOnStreetOnly
+      ) &&
       Objects.equals(transitGeneralizedCostLimit, that.transitGeneralizedCostLimit)
     );
   }
@@ -208,7 +235,9 @@ public int hashCode() {
       nonTransitGeneralizedCostLimit,
       parkAndRideDurationRatio,
       removeItinerariesWithSameRoutesAndStops,
-      transitGeneralizedCostLimit
+      transitGeneralizedCostLimit,
+      removeTransitWithHigherCostThanBestOnStreetOnly,
+      removeTransitWithMoreWalking
     );
   }
 
@@ -227,6 +256,8 @@ public static class Builder {
     private double parkAndRideDurationRatio;
     private boolean removeItinerariesWithSameRoutesAndStops;
     private TransitGeneralizedCostFilterParams transitGeneralizedCostLimit;
+    private CostLinearFunction removeTransitWithHigherCostThanBestOnStreetOnly;
+    private boolean removeTransitWithMoreWalking;
 
     public ItineraryFilterPreferences original() {
       return original;
@@ -302,6 +333,19 @@ public Builder withTransitGeneralizedCostLimit(
       return this;
     }
 
+    public Builder withRemoveTransitWithHigherCostThanBestOnStreetOnly(
+      CostLinearFunction removeTransitWithHigherCostThanBestOnStreetOnly
+    ) {
+      this.removeTransitWithHigherCostThanBestOnStreetOnly =
+        removeTransitWithHigherCostThanBestOnStreetOnly;
+      return this;
+    }
+
+    public Builder withRemoveTransitWithMoreWalking(boolean removeTransitWithMoreWalking) {
+      this.removeTransitWithMoreWalking = removeTransitWithMoreWalking;
+      return this;
+    }
+
     public Builder(ItineraryFilterPreferences original) {
       this.original = original;
       this.accessibilityScore = original.accessibilityScore;
@@ -319,6 +363,9 @@ public Builder(ItineraryFilterPreferences original) {
       this.removeItinerariesWithSameRoutesAndStops =
         original.removeItinerariesWithSameRoutesAndStops;
       this.transitGeneralizedCostLimit = original.transitGeneralizedCostLimit;
+      this.removeTransitWithHigherCostThanBestOnStreetOnly =
+        original.removeTransitWithHigherCostThanBestOnStreetOnly;
+      this.removeTransitWithMoreWalking = original.removeTransitWithMoreWalking;
     }
 
     public Builder apply(Consumer<Builder> body) {