Merge pull request #5856 from ibi-group/flex-duration-factors

Small clean up of flex duration factor/offset

src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java

@@ -21,7 +21,7 @@ class FlexPathTest {
 
   static List<Arguments> cases() {
     return List.of(
-      Arguments.of(TimePenalty.ZERO, THIRTY_MINS_IN_SECONDS),
+      Arguments.of(TimePenalty.NONE, THIRTY_MINS_IN_SECONDS),
       Arguments.of(TimePenalty.of(Duration.ofMinutes(10), 1), 2400),
       Arguments.of(TimePenalty.of(Duration.ofMinutes(10), 1.5f), 3300),
       Arguments.of(TimePenalty.of(Duration.ZERO, 3), 5400)
@@ -30,8 +30,8 @@ static List<Arguments> cases() {
 
   @ParameterizedTest
   @MethodSource("cases")
-  void calculate(TimePenalty mod, int expectedSeconds) {
-    var modified = PATH.withDurationModifier(mod);
+  void calculate(TimePenalty penalty, int expectedSeconds) {
+    var modified = PATH.withTimePenalty(penalty);
     assertEquals(expectedSeconds, modified.durationSeconds);
     assertEquals(LineStrings.SIMPLE, modified.getGeometry());
   }

src/ext-test/java/org/opentripplanner/ext/flex/trip/FlexTripsMapperTest.java

@@ -0,0 +1,37 @@
+package org.opentripplanner.ext.flex.trip;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.opentripplanner.graph_builder.issue.api.DataImportIssueStore.NOOP;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+import org.opentripplanner.ext.flex.FlexStopTimesForTest;
+import org.opentripplanner.ext.flex.FlexTripsMapper;
+import org.opentripplanner.ext.flex.flexpathcalculator.DirectFlexPathCalculator;
+import org.opentripplanner.model.StopTime;
+import org.opentripplanner.model.impl.OtpTransitServiceBuilder;
+import org.opentripplanner.transit.model._data.TransitModelForTest;
+import org.opentripplanner.transit.service.StopModel;
+
+class FlexTripsMapperTest {
+
+  @Test
+  void defaultTimePenalty() {
+    var builder = new OtpTransitServiceBuilder(StopModel.of().build(), NOOP);
+    var stopTimes = List.of(stopTime(0), stopTime(1));
+    builder.getStopTimesSortedByTrip().addAll(stopTimes);
+    var trips = FlexTripsMapper.createFlexTrips(builder, NOOP);
+    assertEquals("[UnscheduledTrip{F:flex-1}]", trips.toString());
+    var unscheduled = (UnscheduledTrip) trips.getFirst();
+    var unchanged = unscheduled.flexPathCalculator(new DirectFlexPathCalculator());
+    assertInstanceOf(DirectFlexPathCalculator.class, unchanged);
+  }
+
+  private static StopTime stopTime(int seq) {
+    var st = FlexStopTimesForTest.area("08:00", "18:00");
+    st.setTrip(TransitModelForTest.trip("flex-1").build());
+    st.setStopSequence(seq);
+    return st;
+  }
+}

src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java

@@ -41,12 +41,8 @@ public LineString getGeometry() {
   /**
    * Returns an (immutable) copy of this path with the duration modified.
    */
-  public FlexPath withDurationModifier(TimePenalty mod) {
-    if (mod.isZero()) {
-      return this;
-    } else {
-      int updatedDuration = (int) mod.calculate(Duration.ofSeconds(durationSeconds)).toSeconds();
-      return new FlexPath(distanceMeters, updatedDuration, geometrySupplier);
-    }
+  public FlexPath withTimePenalty(TimePenalty penalty) {
+    int updatedDuration = (int) penalty.calculate(Duration.ofSeconds(durationSeconds)).toSeconds();
+    return new FlexPath(distanceMeters, updatedDuration, geometrySupplier);
   }
 }

src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java

@@ -5,17 +5,17 @@
 import org.opentripplanner.street.model.vertex.Vertex;
 
 /**
- * A calculator to delegates the main computation to another instance and applies a duration
- * modifier afterward.
+ * A calculator to delegates the main computation to another instance and applies a time penalty
+ * afterward.
  */
 public class TimePenaltyCalculator implements FlexPathCalculator {
 
   private final FlexPathCalculator delegate;
-  private final TimePenalty factors;
+  private final TimePenalty penalty;
 
   public TimePenaltyCalculator(FlexPathCalculator delegate, TimePenalty penalty) {
     this.delegate = delegate;
-    this.factors = penalty;
+    this.penalty = penalty;
   }
 
   @Nullable
@@ -26,7 +26,7 @@ public FlexPath calculateFlexPath(Vertex fromv, Vertex tov, int fromStopIndex, i
     if (path == null) {
       return null;
     } else {
-      return path.withDurationModifier(factors);
+      return path.withTimePenalty(penalty);
     }
   }
 }

src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java

@@ -156,7 +156,7 @@ public Stream<FlexAccessTemplate> getFlexAccessTemplates(
 
   /**
    * Get the correct {@link FlexPathCalculator} depending on the {@code timePenalty}.
-   * If the modifier doesn't actually modify, we return the regular calculator.
+   * If the penalty would not change the result, we return the regular calculator.
    */
   protected FlexPathCalculator flexPathCalculator(FlexPathCalculator calculator) {
     if (timePenalty.modifies()) {

src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java

@@ -171,7 +171,7 @@ public void mapStopTripAndRouteDataIntoBuilder() {
 
     builder.getPathways().addAll(pathwayMapper.map(data.getAllPathways()));
     builder.getStopTimesSortedByTrip().addAll(stopTimeMapper.map(data.getAllStopTimes()));
-    builder.getFlexTimePenalty().putAll(tripMapper.flexSafeDurationModifiers());
+    builder.getFlexTimePenalty().putAll(tripMapper.flexSafeTimePenalties());
     builder.getTripsById().addAll(tripMapper.map(data.getAllTrips()));
 
     fareRulesBuilder.fareAttributes().addAll(fareAttributeMapper.map(data.getAllFareAttributes()));

src/main/java/org/opentripplanner/gtfs/mapping/TripMapper.java

@@ -18,7 +18,7 @@ class TripMapper {
   private final TranslationHelper translationHelper;
 
   private final Map<org.onebusaway.gtfs.model.Trip, Trip> mappedTrips = new HashMap<>();
-  private final Map<Trip, TimePenalty> flexSafeDurationModifiers = new HashMap<>();
+  private final Map<Trip, TimePenalty> flexSafeTimePenalties = new HashMap<>();
 
   TripMapper(
     RouteMapper routeMapper,
@@ -45,8 +45,8 @@ Collection<Trip> getMappedTrips() {
   /**
    * The map of flex duration factors per flex trip.
    */
-  Map<Trip, TimePenalty> flexSafeDurationModifiers() {
-    return flexSafeDurationModifiers;
+  Map<Trip, TimePenalty> flexSafeTimePenalties() {
+    return flexSafeTimePenalties;
   }
 
   private Trip doMap(org.onebusaway.gtfs.model.Trip rhs) {
@@ -73,11 +73,11 @@ private Trip doMap(org.onebusaway.gtfs.model.Trip rhs) {
     lhs.withBikesAllowed(BikeAccessMapper.mapForTrip(rhs));
 
     var trip = lhs.build();
-    mapSafeDurationModifier(rhs).ifPresent(f -> flexSafeDurationModifiers.put(trip, f));
+    mapSafeTimePenalty(rhs).ifPresent(f -> flexSafeTimePenalties.put(trip, f));
     return trip;
   }
 
-  private Optional<TimePenalty> mapSafeDurationModifier(org.onebusaway.gtfs.model.Trip rhs) {
+  private Optional<TimePenalty> mapSafeTimePenalty(org.onebusaway.gtfs.model.Trip rhs) {
     if (rhs.getSafeDurationFactor() == null && rhs.getSafeDurationOffset() == null) {
       return Optional.empty();
     } else {

src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java

@@ -94,7 +94,7 @@ public class OtpTransitServiceBuilder {
 
   private final TripStopTimes stopTimesByTrip = new TripStopTimes();
 
-  private final Map<Trip, TimePenalty> flexDurationFactors = new HashMap<>();
+  private final Map<Trip, TimePenalty> flexTimePenalties = new HashMap<>();
 
   private final EntityById<FareZone> fareZonesById = new DefaultEntityById<>();
 
@@ -214,7 +214,7 @@ public TripStopTimes getStopTimesSortedByTrip() {
   }
 
   public Map<Trip, TimePenalty> getFlexTimePenalty() {
-    return flexDurationFactors;
+    return flexTimePenalties;
   }
 
   public EntityById<FareZone> getFareZonesById() {

src/main/java/org/opentripplanner/routing/api/request/framework/TimePenalty.java

@@ -7,6 +7,9 @@
 public final class TimePenalty extends AbstractLinearFunction<Duration> {
 
   public static final TimePenalty ZERO = new TimePenalty(Duration.ZERO, 0.0);
+  /**
+   * An instance that doesn't actually apply a penalty and returns the duration unchanged.
+   */
   public static final TimePenalty NONE = new TimePenalty(Duration.ZERO, 1.0);
 
   private TimePenalty(Duration constant, double coefficient) {

src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java

@@ -111,23 +111,23 @@ void testMapCache() throws Exception {
   }
 
   @Test
-  void noFlexDurationModifier() {
+  void noFlexTimePenalty() {
     var mapper = defaultTripMapper();
     mapper.map(TRIP);
-    assertTrue(mapper.flexSafeDurationModifiers().isEmpty());
+    assertTrue(mapper.flexSafeTimePenalties().isEmpty());
   }
 
   @Test
-  void flexDurationModifier() {
+  void flexTimePenalty() {
     var flexTrip = new Trip();
     flexTrip.setId(new AgencyAndId("1", "1"));
     flexTrip.setSafeDurationFactor(1.5);
     flexTrip.setSafeDurationOffset(600d);
     flexTrip.setRoute(new GtfsTestData().route);
     var mapper = defaultTripMapper();
     var mapped = mapper.map(flexTrip);
-    var mod = mapper.flexSafeDurationModifiers().get(mapped);
-    assertEquals(1.5f, mod.coefficient());
-    assertEquals(600, mod.constant().toSeconds());
+    var penalty = mapper.flexSafeTimePenalties().get(mapped);
+    assertEquals(1.5f, penalty.coefficient());
+    assertEquals(600, penalty.constant().toSeconds());
   }
 }