From 1fbf5ca42445b6ee3fc483394be8dcfda48a70ba Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Sat, 18 May 2024 09:33:54 +0200 Subject: [PATCH 1/4] Finish up renaming duration factor to time penalty --- .../ext/flex/flexpathcalculator/FlexPathTest.java | 4 ++-- .../ext/flex/flexpathcalculator/FlexPath.java | 2 +- .../flex/flexpathcalculator/TimePenaltyCalculator.java | 6 +++--- .../gtfs/mapping/GTFSToOtpTransitServiceMapper.java | 2 +- .../org/opentripplanner/gtfs/mapping/TripMapper.java | 10 +++++----- .../model/impl/OtpTransitServiceBuilder.java | 4 ++-- .../opentripplanner/gtfs/mapping/TripMapperTest.java | 8 ++++---- 7 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java index 8bd3abee785..fca0d22cdf3 100644 --- a/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java @@ -30,8 +30,8 @@ static List 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()); } diff --git a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java index 3a692dff40b..0b227490e9d 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java +++ b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java @@ -41,7 +41,7 @@ public LineString getGeometry() { /** * Returns an (immutable) copy of this path with the duration modified. */ - public FlexPath withDurationModifier(TimePenalty mod) { + public FlexPath withTimePenalty(TimePenalty mod) { if (mod.isZero()) { return this; } else { diff --git a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java index 63b661f0f9a..4fde3fcfbff 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java +++ b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java @@ -11,11 +11,11 @@ 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); } } } diff --git a/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java b/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java index ce65d6b0820..354c1f16177 100644 --- a/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java +++ b/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())); diff --git a/src/main/java/org/opentripplanner/gtfs/mapping/TripMapper.java b/src/main/java/org/opentripplanner/gtfs/mapping/TripMapper.java index ca221c1c6ca..3a62ba2b269 100644 --- a/src/main/java/org/opentripplanner/gtfs/mapping/TripMapper.java +++ b/src/main/java/org/opentripplanner/gtfs/mapping/TripMapper.java @@ -18,7 +18,7 @@ class TripMapper { private final TranslationHelper translationHelper; private final Map mappedTrips = new HashMap<>(); - private final Map flexSafeDurationModifiers = new HashMap<>(); + private final Map flexSafeTimePenalties = new HashMap<>(); TripMapper( RouteMapper routeMapper, @@ -45,8 +45,8 @@ Collection getMappedTrips() { /** * The map of flex duration factors per flex trip. */ - Map flexSafeDurationModifiers() { - return flexSafeDurationModifiers; + Map 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 mapSafeDurationModifier(org.onebusaway.gtfs.model.Trip rhs) { + private Optional mapSafeTimePenalty(org.onebusaway.gtfs.model.Trip rhs) { if (rhs.getSafeDurationFactor() == null && rhs.getSafeDurationOffset() == null) { return Optional.empty(); } else { diff --git a/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java b/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java index 544ca29599d..43c18cec59d 100644 --- a/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java +++ b/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 flexDurationFactors = new HashMap<>(); + private final Map flexTimePenalties = new HashMap<>(); private final EntityById fareZonesById = new DefaultEntityById<>(); @@ -214,7 +214,7 @@ public TripStopTimes getStopTimesSortedByTrip() { } public Map getFlexTimePenalty() { - return flexDurationFactors; + return flexTimePenalties; } public EntityById getFareZonesById() { diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java index f24e405515d..535ec349fba 100644 --- a/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java +++ b/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java @@ -111,14 +111,14 @@ 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); @@ -126,7 +126,7 @@ void flexDurationModifier() { flexTrip.setRoute(new GtfsTestData().route); var mapper = defaultTripMapper(); var mapped = mapper.map(flexTrip); - var mod = mapper.flexSafeDurationModifiers().get(mapped); + var mod = mapper.flexSafeTimePenalties().get(mapped); assertEquals(1.5f, mod.coefficient()); assertEquals(600, mod.constant().toSeconds()); } From 0113169c6b7cd2b6c10dcfb6278f4bcfc2734f46 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Sat, 18 May 2024 14:10:26 +0200 Subject: [PATCH 2/4] Add test for default time penalty --- .../ext/flex/FlexTripsMapperTest.java | 30 +++++++++++++++++++ .../flex/flexpathcalculator/FlexPathTest.java | 2 +- .../ext/flex/flexpathcalculator/FlexPath.java | 10 ++----- .../ext/flex/trip/UnscheduledTrip.java | 12 +++++++- 4 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java new file mode 100644 index 00000000000..9670bec2802 --- /dev/null +++ b/src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java @@ -0,0 +1,30 @@ +package org.opentripplanner.ext.flex; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.opentripplanner.graph_builder.issue.api.DataImportIssueStore.NOOP; + +import java.util.List; +import org.junit.jupiter.api.Test; +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 timePenalty=(0s + 1.00 t)}]", trips.toString()); + } + + 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; + } +} diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java index fca0d22cdf3..3d37de02af4 100644 --- a/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPathTest.java @@ -21,7 +21,7 @@ class FlexPathTest { static List 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) diff --git a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java index 0b227490e9d..4a494286313 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/FlexPath.java +++ b/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 withTimePenalty(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); } } diff --git a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java index 402d39e2aa7..a14f80d74a4 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java +++ b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java @@ -14,6 +14,7 @@ import java.util.stream.IntStream; import java.util.stream.Stream; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import org.opentripplanner.ext.flex.FlexServiceDate; import org.opentripplanner.ext.flex.flexpathcalculator.FlexPathCalculator; import org.opentripplanner.ext.flex.flexpathcalculator.TimePenaltyCalculator; @@ -29,6 +30,7 @@ import org.opentripplanner.routing.graphfinder.NearbyStop; import org.opentripplanner.standalone.config.sandbox.FlexConfig; import org.opentripplanner.transit.model.framework.FeedScopedId; +import org.opentripplanner.transit.model.framework.LogInfo; import org.opentripplanner.transit.model.framework.TransitBuilder; import org.opentripplanner.transit.model.site.GroupStop; import org.opentripplanner.transit.model.site.StopLocation; @@ -45,7 +47,9 @@ *

* For a discussion of this behaviour see https://github.com/MobilityData/gtfs-flex/issues/76 */ -public class UnscheduledTrip extends FlexTrip { +public class UnscheduledTrip + extends FlexTrip + implements LogInfo { private static final Set N_STOPS = Set.of(1, 2); private static final int INDEX_NOT_FOUND = -1; @@ -154,6 +158,12 @@ public Stream getFlexAccessTemplates( ); } + @Nullable + @Override + public String logName() { + return "timePenalty=(%s)".formatted(timePenalty.toString()); + } + /** * Get the correct {@link FlexPathCalculator} depending on the {@code timePenalty}. * If the modifier doesn't actually modify, we return the regular calculator. From 57c4282909bc05ef1eee427954639d780b2e2659 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 22 May 2024 09:21:51 +0200 Subject: [PATCH 3/4] Apply review feedback --- .../ext/flex/flexpathcalculator/TimePenaltyCalculator.java | 4 ++-- .../org/opentripplanner/ext/flex/trip/UnscheduledTrip.java | 2 +- .../routing/api/request/framework/TimePenalty.java | 3 +++ .../org/opentripplanner/gtfs/mapping/TripMapperTest.java | 6 +++--- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java index 4fde3fcfbff..a2252f3fec8 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java +++ b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/TimePenaltyCalculator.java @@ -5,8 +5,8 @@ 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 { diff --git a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java index a14f80d74a4..08c1f9b5d3c 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java +++ b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java @@ -166,7 +166,7 @@ public String logName() { /** * 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()) { diff --git a/src/main/java/org/opentripplanner/routing/api/request/framework/TimePenalty.java b/src/main/java/org/opentripplanner/routing/api/request/framework/TimePenalty.java index 0c6fdd96436..3ee07034135 100644 --- a/src/main/java/org/opentripplanner/routing/api/request/framework/TimePenalty.java +++ b/src/main/java/org/opentripplanner/routing/api/request/framework/TimePenalty.java @@ -7,6 +7,9 @@ public final class TimePenalty extends AbstractLinearFunction { 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) { diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java index 535ec349fba..964c3d8155e 100644 --- a/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java +++ b/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java @@ -126,8 +126,8 @@ void flexTimePenalty() { flexTrip.setRoute(new GtfsTestData().route); var mapper = defaultTripMapper(); var mapped = mapper.map(flexTrip); - var mod = mapper.flexSafeTimePenalties().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()); } } From 3faf5916abcea6c2440fcde7054729fcb18c624d Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Thu, 23 May 2024 17:26:53 +0200 Subject: [PATCH 4/4] Re-implement test by checking the calculator --- .../ext/flex/{ => trip}/FlexTripsMapperTest.java | 11 +++++++++-- .../ext/flex/trip/UnscheduledTrip.java | 12 +----------- 2 files changed, 10 insertions(+), 13 deletions(-) rename src/ext-test/java/org/opentripplanner/ext/flex/{ => trip}/FlexTripsMapperTest.java (62%) diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/trip/FlexTripsMapperTest.java similarity index 62% rename from src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java rename to src/ext-test/java/org/opentripplanner/ext/flex/trip/FlexTripsMapperTest.java index 9670bec2802..0d0376bbd32 100644 --- a/src/ext-test/java/org/opentripplanner/ext/flex/FlexTripsMapperTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/flex/trip/FlexTripsMapperTest.java @@ -1,10 +1,14 @@ -package org.opentripplanner.ext.flex; +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; @@ -18,7 +22,10 @@ void defaultTimePenalty() { var stopTimes = List.of(stopTime(0), stopTime(1)); builder.getStopTimesSortedByTrip().addAll(stopTimes); var trips = FlexTripsMapper.createFlexTrips(builder, NOOP); - assertEquals("[UnscheduledTrip{F:flex-1 timePenalty=(0s + 1.00 t)}]", trips.toString()); + 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) { diff --git a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java index 08c1f9b5d3c..a4c8d9568ca 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java +++ b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java @@ -14,7 +14,6 @@ import java.util.stream.IntStream; import java.util.stream.Stream; import javax.annotation.Nonnull; -import javax.annotation.Nullable; import org.opentripplanner.ext.flex.FlexServiceDate; import org.opentripplanner.ext.flex.flexpathcalculator.FlexPathCalculator; import org.opentripplanner.ext.flex.flexpathcalculator.TimePenaltyCalculator; @@ -30,7 +29,6 @@ import org.opentripplanner.routing.graphfinder.NearbyStop; import org.opentripplanner.standalone.config.sandbox.FlexConfig; import org.opentripplanner.transit.model.framework.FeedScopedId; -import org.opentripplanner.transit.model.framework.LogInfo; import org.opentripplanner.transit.model.framework.TransitBuilder; import org.opentripplanner.transit.model.site.GroupStop; import org.opentripplanner.transit.model.site.StopLocation; @@ -47,9 +45,7 @@ *

* For a discussion of this behaviour see https://github.com/MobilityData/gtfs-flex/issues/76 */ -public class UnscheduledTrip - extends FlexTrip - implements LogInfo { +public class UnscheduledTrip extends FlexTrip { private static final Set N_STOPS = Set.of(1, 2); private static final int INDEX_NOT_FOUND = -1; @@ -158,12 +154,6 @@ public Stream getFlexAccessTemplates( ); } - @Nullable - @Override - public String logName() { - return "timePenalty=(%s)".formatted(timePenalty.toString()); - } - /** * Get the correct {@link FlexPathCalculator} depending on the {@code timePenalty}. * If the penalty would not change the result, we return the regular calculator.