Merge pull request opentripplanner#6656 from Skanetrafiken/fix-negative-cost-in-filter-2

Fix negative cost in filter

Author: habrahamsson-skanetrafiken

application/src/main/java/org/opentripplanner/routing/algorithm/filterchain/filters/transit/group/RemoveOtherThanSameLegsMaxGeneralizedCost.java

@@ -17,6 +17,22 @@
  * itineraries have a much higher cost for the other legs. This is similar to {@link
  * org.opentripplanner.routing.algorithm.filterchain.filters.transit.TransitGeneralizedCostFilter},
  * but is used together with {@link GroupByFilter} to filter within the groups.
+ *
+ * <h3>Example</h3>
+ *
+ * Lets give 5% slack: f=1.05
+ *
+ * <pre>
+ * Itin A:  | ### cost of legs common trips - $42 ### | ########## other legs - $41 ########## |    (Total: $83)
+ * Itin B:  | ######## cost of legs common trips - $52 ######## | ### other legs - $27 ### |        (Total: $79)
+ * </pre>
+ *
+ * <ul>
+ * <li>Min cost common legs: a=$42</li>
+ * <li>Min cost all itineraries: b=$79</li>
+ * <li>maxLimit = a + (b - a) * f = 42 + 37 * 1.05 = 81</li>
+ * <li><b>Result:</b> Keep itinerary A, and drop B ($83 > limit $81)</li>
+ * </ul>
  */
 public class RemoveOtherThanSameLegsMaxGeneralizedCost implements RemoveItineraryFlagger {
 
@@ -26,6 +42,9 @@ public class RemoveOtherThanSameLegsMaxGeneralizedCost implements RemoveItinerar
   private final double maxCostOtherLegsFactor;
 
   public RemoveOtherThanSameLegsMaxGeneralizedCost(double maxCostOtherLegsFactor) {
+    if (maxCostOtherLegsFactor < 1.0) {
+      throw new IllegalArgumentException("maxCostOtherLegsFactor must be >= 1.0");
+    }
     this.maxCostOtherLegsFactor = maxCostOtherLegsFactor;
   }
 
@@ -59,35 +78,39 @@ public List<Itinerary> flagForRemoval(List<Itinerary> itineraries) {
       })
       .get();
 
+    if (commonTrips.isEmpty()) {
+      return List.of();
+    }
+
     // Find the lowest cost of the common legs
-    OptionalInt commonCost = itineraries
+    int commonLegsCost = itineraries
       .stream()
       .mapToInt(itinerary ->
         itinerary
           .legs()
           .stream()
           .filter(Leg::isTransitLeg)
           .filter(leg -> commonTrips.contains(leg.trip()))
-          .mapToInt(leg -> leg.generalizedCost())
+          .mapToInt(leg -> Integer.max(0, leg.generalizedCost()))
           .sum()
       )
-      .min();
-
-    if (commonCost.isEmpty()) {
-      return List.of();
-    }
+      .min()
+      .orElseThrow();
 
     // Find the lowest cost for any itinerary
-    int minimumCost = itineraries
+    int minimumItineraryCost = itineraries
       .stream()
       .mapToInt(it -> it.generalizedCostIncludingPenalty().toSeconds())
       .min()
       .orElseThrow();
 
+    // Leg costs in otp are not guaranteed to be accurate and it is possible that the sum of the
+    // commonLegCosts are higher than the minimumItineraryCost. So we need to make sure this does
+    // not go negative.
+    int otherLegsCost = Integer.max(0, minimumItineraryCost - commonLegsCost);
+
     // Calculate the maximum limit allowed for itinerary cost
-    Cost maxLimit = Cost.costOfSeconds(
-      ((minimumCost - commonCost.getAsInt()) * maxCostOtherLegsFactor) + commonCost.getAsInt()
-    );
+    Cost maxLimit = Cost.costOfSeconds(otherLegsCost * maxCostOtherLegsFactor + commonLegsCost);
 
     return itineraries
       .stream()

application/src/test/java/org/opentripplanner/model/plan/TestItineraryBuilder.java

@@ -303,7 +303,13 @@ public TestItineraryBuilder bus(
   /**
    * Add a rail/train leg to the itinerary
    */
-  public TestItineraryBuilder rail(int tripId, int startTime, int endTime, Place to) {
+  public TestItineraryBuilder rail(
+    int tripId,
+    int startTime,
+    int endTime,
+    Place to,
+    @Nullable Integer cost
+  ) {
     return transit(
       RAIL_ROUTE,
       Integer.toString(tripId),
@@ -314,10 +320,15 @@ public TestItineraryBuilder rail(int tripId, int startTime, int endTime, Place t
       to,
       null,
       null,
-      null
+      null,
+      cost
     );
   }
 
+  public TestItineraryBuilder rail(int tripId, int startTime, int endTime, Place to) {
+    return rail(tripId, startTime, endTime, to, null);
+  }
+
   public TestItineraryBuilder faresV2Rail(
     int tripId,
     int startTime,
@@ -464,14 +475,46 @@ public TestItineraryBuilder transit(
     LocalDate serviceDate,
     Integer headwaySecs,
     ConstrainedTransfer transferFromPreviousLeg
+  ) {
+    return transit(
+      route,
+      tripId,
+      start,
+      end,
+      fromStopIndex,
+      toStopIndex,
+      to,
+      serviceDate,
+      headwaySecs,
+      transferFromPreviousLeg,
+      null
+    );
+  }
+
+  public TestItineraryBuilder transit(
+    Route route,
+    String tripId,
+    int start,
+    int end,
+    int fromStopIndex,
+    int toStopIndex,
+    Place to,
+    LocalDate serviceDate,
+    Integer headwaySecs,
+    ConstrainedTransfer transferFromPreviousLeg,
+    @Nullable Integer cost
   ) {
     if (lastPlace == null) {
       throw new IllegalStateException("Trip from place is unknown!");
     }
     int waitTime = start - lastEndTime(start);
     int legCost = 0;
-    legCost += cost(WAIT_RELUCTANCE_FACTOR, waitTime);
-    legCost += cost(1.0f, end - start) + BOARD_COST;
+    if (cost != null) {
+      legCost = cost;
+    } else {
+      legCost += cost(WAIT_RELUCTANCE_FACTOR, waitTime);
+      legCost += cost(1.0f, end - start) + BOARD_COST;
+    }
 
     Trip trip = trip(tripId, route);
 

application/src/test/java/org/opentripplanner/routing/algorithm/filterchain/filters/transit/group/RemoveOtherThanSameLegsMaxGeneralizedCostTest.java

@@ -22,4 +22,36 @@ public void testFilter() {
     var subject = new RemoveOtherThanSameLegsMaxGeneralizedCost(2.0);
     assertEquals(List.of(second), subject.flagForRemoval(List.of(first, second)));
   }
+
+  @Test
+  public void testFilterNothingInCommon() {
+    Itinerary first = newItinerary(A)
+      .rail(20, T11_05, T11_14, B)
+      .bus(30, T11_16, T11_20, C)
+      .build();
+
+    Itinerary second = newItinerary(A).rail(40, T11_05, T11_14, B).walk(D10m, C).build();
+
+    var subject = new RemoveOtherThanSameLegsMaxGeneralizedCost(2.0);
+    assertEquals(List.of(), subject.flagForRemoval(List.of(first, second)));
+  }
+
+  @Test
+  public void testFilterInaccurateLegCosts() {
+    // Regression test that verifies that we don't crash in the case where the sum of the leg costs
+    // is larger than the itinerary cost.
+    int itineraryCost = 100;
+    Itinerary first = newItinerary(A)
+      .rail(20, T11_05, T11_14, B, 400)
+      .walk(60 * 4, C)
+      .build(itineraryCost);
+
+    Itinerary second = newItinerary(A)
+      .rail(20, T11_05, T11_14, B, 400)
+      .walk(D10m, C)
+      .build(itineraryCost);
+
+    var subject = new RemoveOtherThanSameLegsMaxGeneralizedCost(2.0);
+    assertEquals(List.of(), subject.flagForRemoval(List.of(first, second)));
+  }
 }