From 991406ed25845660bd0a25e972e86c66227b7409 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 9 Apr 2024 13:42:44 +0200 Subject: [PATCH] Cleanup, tests and documentation --- doc-templates/Flex.md | 4 +- docs/sandbox/Flex.md | 4 +- .../flex/flexpathcalculator/FlexPathTest.java | 1 + .../ext/flex/trip/DurationModifierTest.java | 2 +- .../trip/UnscheduledDrivingDurationTest.java | 44 ++++++++++++++++++ .../ext/flex/trip/UnscheduledTripTest.java | 11 ++--- .../ext/flex/FlexTripsMapper.java | 6 +-- .../DurationModifierCalculator.java | 4 ++ .../ext/flex/trip/ScheduledDeviatedTrip.java | 10 ----- .../ext/flex/trip/UnscheduledTrip.java | 6 ++- .../GTFSToOtpTransitServiceMapper.java | 2 +- .../gtfs/mapping/TripMapper.java | 10 ++--- .../gtfs/mapping/TripMapperTest.java | 45 ++++++++++++++----- 13 files changed, 107 insertions(+), 42 deletions(-) create mode 100644 src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledDrivingDurationTest.java diff --git a/doc-templates/Flex.md b/doc-templates/Flex.md index 2015e898cae..50512f66133 100644 --- a/doc-templates/Flex.md +++ b/doc-templates/Flex.md @@ -10,8 +10,8 @@ To enable this turn on `FlexRouting` as a feature in `otp-config.json`. -The GTFS feeds should conform to the -[GTFS-Flex v2 draft PR](https://github.com/google/transit/pull/388) +The GTFS feeds must conform to the final, approved version of the draft which has been +merged into the [mainline specification](https://gtfs.org/schedule/reference/) in March 2024. ## Configuration diff --git a/docs/sandbox/Flex.md b/docs/sandbox/Flex.md index 61d15851a56..277e4e617f2 100644 --- a/docs/sandbox/Flex.md +++ b/docs/sandbox/Flex.md @@ -10,8 +10,8 @@ To enable this turn on `FlexRouting` as a feature in `otp-config.json`. -The GTFS feeds should conform to the -[GTFS-Flex v2 draft PR](https://github.com/google/transit/pull/388) +The GTFS feeds must conform to the final, approved version of the draft which has been +merged into the [mainline specification](https://gtfs.org/schedule/reference/) in March 2024. ## Configuration 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 6a4c79ca2cc..b79093e26c5 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 @@ -33,5 +33,6 @@ static List cases() { void calculate(DurationModifier mod, int expectedSeconds) { var modified = PATH.withDurationModifier(mod); assertEquals(expectedSeconds, modified.durationSeconds); + assertEquals(LineStrings.SIMPLE, modified.getGeometry()); } } diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/trip/DurationModifierTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/trip/DurationModifierTest.java index c6f4e75f7c5..05fb9965935 100644 --- a/src/ext-test/java/org/opentripplanner/ext/flex/trip/DurationModifierTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/flex/trip/DurationModifierTest.java @@ -17,4 +17,4 @@ void doesNotModify() { void modifies() { assertTrue(new DurationModifier(Duration.ofMinutes(1), 1.5f).modifies()); } -} \ No newline at end of file +} diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledDrivingDurationTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledDrivingDurationTest.java new file mode 100644 index 00000000000..45d8c5be624 --- /dev/null +++ b/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledDrivingDurationTest.java @@ -0,0 +1,44 @@ +package org.opentripplanner.ext.flex.trip; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.opentripplanner.street.model._data.StreetModelForTest.V1; +import static org.opentripplanner.street.model._data.StreetModelForTest.V2; +import static org.opentripplanner.transit.model._data.TransitModelForTest.id; + +import java.time.Duration; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.opentripplanner._support.geometry.LineStrings; +import org.opentripplanner.ext.flex.FlexStopTimesForTest; +import org.opentripplanner.ext.flex.flexpathcalculator.FlexPath; +import org.opentripplanner.ext.flex.flexpathcalculator.FlexPathCalculator; +import org.opentripplanner.model.StopTime; + +class UnscheduledDrivingDurationTest { + + static final FlexPathCalculator STATIC_CALCULATOR = (fromv, tov, fromStopIndex, toStopIndex) -> + new FlexPath(10_000, (int) Duration.ofMinutes(10).toSeconds(), () -> LineStrings.SIMPLE); + private static final StopTime STOP_TIME = FlexStopTimesForTest.area("10:00", "18:00"); + + @Test + void noModifier() { + var trip = UnscheduledTrip.of(id("1")).withStopTimes(List.of(STOP_TIME)).build(); + + var calculator = trip.flexPathCalculator(STATIC_CALCULATOR); + var path = calculator.calculateFlexPath(V1, V2, 0, 0); + assertEquals(600, path.durationSeconds); + } + + @Test + void withModifier() { + var trip = UnscheduledTrip + .of(id("1")) + .withStopTimes(List.of(STOP_TIME)) + .withDurationModifier(new DurationModifier(Duration.ofMinutes(2), 1.5f)) + .build(); + + var calculator = trip.flexPathCalculator(STATIC_CALCULATOR); + var path = calculator.calculateFlexPath(V1, V2, 0, 0); + assertEquals(1020, path.durationSeconds); + } +} diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledTripTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledTripTest.java index bfcd9bad565..80bda31fabf 100644 --- a/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledTripTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/flex/trip/UnscheduledTripTest.java @@ -4,6 +4,8 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.opentripplanner.ext.flex.FlexStopTimesForTest.area; +import static org.opentripplanner.ext.flex.FlexStopTimesForTest.regularArrival; +import static org.opentripplanner.ext.flex.FlexStopTimesForTest.regularDeparture; import static org.opentripplanner.ext.flex.trip.UnscheduledTrip.isUnscheduledTrip; import static org.opentripplanner.ext.flex.trip.UnscheduledTripTest.TestCase.tc; import static org.opentripplanner.model.PickDrop.NONE; @@ -23,7 +25,6 @@ import org.junit.jupiter.params.provider.MethodSource; import org.opentripplanner._support.geometry.Polygons; import org.opentripplanner.ext.flex.FlexServiceDate; -import org.opentripplanner.ext.flex.FlexStopTimesForTest; import org.opentripplanner.ext.flex.flexpathcalculator.DirectFlexPathCalculator; import org.opentripplanner.ext.flex.template.FlexAccessTemplate; import org.opentripplanner.ext.flex.template.FlexEgressTemplate; @@ -197,7 +198,7 @@ void testUnscheduledFeederTripToScheduledStop() { static Stream testRegularStopToAreaEarliestDepartureTimeTestCases() { // REGULAR-STOP to AREA - (10:00-14:00) => (14:00) - var tc = tc(FlexStopTimesForTest.regularDeparture("10:00"), area("10:00", "14:00")); + var tc = tc(regularDeparture("10:00"), area("10:00", "14:00")); return Stream.of( tc .expected("Requested departure time is before flex service departure time", "10:00") @@ -246,7 +247,7 @@ void testRegularStopToAreaEarliestDepartureTime(TestCase tc) { static Stream testAreaToRegularStopEarliestDepartureTestCases() { // AREA TO REGULAR-STOP - (10:00-14:00) => (14:00) - var tc = tc(area("10:00", "14:00"), FlexStopTimesForTest.regularArrival("14:00")); + var tc = tc(area("10:00", "14:00"), regularArrival("14:00")); return Stream.of( tc .expected( @@ -367,7 +368,7 @@ void testAreaToAreaEarliestDepartureTime(TestCase tc) { static Stream testRegularStopToAreaLatestArrivalTimeTestCases() { // REGULAR-STOP to AREA - (10:00-14:00) => (14:00) - var tc = tc(FlexStopTimesForTest.regularDeparture("10:00"), area("10:00", "14:00")); + var tc = tc(regularDeparture("10:00"), area("10:00", "14:00")); return Stream.of( tc .expectedNotFound("Requested arrival time is before flex service arrival window start") @@ -422,7 +423,7 @@ void testRegularStopToAreaLatestArrivalTime(TestCase tc) { static Stream testAreaToRegularStopLatestArrivalTimeTestCases() { // AREA TO REGULAR-STOP - (10:00-14:00) => (14:00) - var tc = tc(area("10:00", "14:00"), FlexStopTimesForTest.regularArrival("14:00")); + var tc = tc(area("10:00", "14:00"), regularArrival("14:00")); return Stream.of( tc .expectedNotFound("Requested arrival time is before flex service arrival window start") diff --git a/src/ext/java/org/opentripplanner/ext/flex/FlexTripsMapper.java b/src/ext/java/org/opentripplanner/ext/flex/FlexTripsMapper.java index b4123a3b556..bb2ed2764f6 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/FlexTripsMapper.java +++ b/src/ext/java/org/opentripplanner/ext/flex/FlexTripsMapper.java @@ -47,11 +47,7 @@ public class FlexTripsMapper { ); } else if (ScheduledDeviatedTrip.isScheduledFlexTrip(stopTimes)) { result.add( - ScheduledDeviatedTrip - .of(trip.getId()) - .withTrip(trip) - .withStopTimes(stopTimes) - .build() + ScheduledDeviatedTrip.of(trip.getId()).withTrip(trip).withStopTimes(stopTimes).build() ); } else if (hasContinuousStops(stopTimes) && FlexTrip.containsFlexStops(stopTimes)) { store.add( diff --git a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/DurationModifierCalculator.java b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/DurationModifierCalculator.java index 643c2bb7b6f..158e6a933c7 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/DurationModifierCalculator.java +++ b/src/ext/java/org/opentripplanner/ext/flex/flexpathcalculator/DurationModifierCalculator.java @@ -4,6 +4,10 @@ import org.opentripplanner.ext.flex.trip.DurationModifier; import org.opentripplanner.street.model.vertex.Vertex; +/** + * A calculator to delegates the main computation to another instance and applies a duration + * modifier afterward. + */ public class DurationModifierCalculator implements FlexPathCalculator { private final FlexPathCalculator delegate; diff --git a/src/ext/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTrip.java b/src/ext/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTrip.java index 84982c837dc..0026d98c964 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTrip.java +++ b/src/ext/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTrip.java @@ -1,7 +1,5 @@ package org.opentripplanner.ext.flex.trip; -import static org.opentripplanner.ext.flex.trip.ScheduledDeviatedTrip.TimeType.FIXED_TIME; -import static org.opentripplanner.ext.flex.trip.ScheduledDeviatedTrip.TimeType.FLEXIBLE_TIME; import static org.opentripplanner.model.PickDrop.NONE; import static org.opentripplanner.model.StopTime.MISSING_VALUE; @@ -299,13 +297,10 @@ private static class ScheduledDeviatedStopTime implements Serializable { private final int arrivalTime; private final PickDrop pickupType; private final PickDrop dropOffType; - private final TimeType timeType; private ScheduledDeviatedStopTime(StopTime st) { this.stop = st.getStop(); - this.timeType = st.hasFlexWindow() ? FLEXIBLE_TIME : FIXED_TIME; - // Store the time the user is guaranteed to arrive at latest this.arrivalTime = st.getLatestPossibleArrivalTime(); // Store the time the user needs to be ready for pickup @@ -320,9 +315,4 @@ private ScheduledDeviatedStopTime(StopTime st) { this.dropOffType = arrivalTime == MISSING_VALUE ? PickDrop.NONE : st.getDropOffType(); } } - - enum TimeType { - FIXED_TIME, - FLEXIBLE_TIME, - } } 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 d8e942d0c98..bc3bd52cc4c 100644 --- a/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java +++ b/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java @@ -149,7 +149,11 @@ public Stream getFlexAccessTemplates( ); } - private FlexPathCalculator flexPathCalculator(FlexPathCalculator calculator) { + /** + * Get the correct {@link FlexPathCalculator} depending on the {@code durationModified}. + * If the modifier doesn't actually modify, we don't + */ + protected FlexPathCalculator flexPathCalculator(FlexPathCalculator calculator) { if (durationModifier.modifies()) { return new DurationModifierCalculator(calculator, durationModifier); } else { diff --git a/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java b/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java index 4ee1cdcf1d7..24db367f829 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.getFlexDurationFactors().putAll(tripMapper.flexSafeDurationFactors()); + builder.getFlexDurationFactors().putAll(tripMapper.flexSafeDurationModifiers()); 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 cc6e44343e6..ec7d1379ca0 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 flexSafeDurationFactors = new HashMap<>(); + private final Map flexSafeDurationModifiers = new HashMap<>(); TripMapper( RouteMapper routeMapper, @@ -45,8 +45,8 @@ Collection getMappedTrips() { /** * The map of flex duration factors per flex trip. */ - Map flexSafeDurationFactors() { - return flexSafeDurationFactors; + Map flexSafeDurationModifiers() { + return flexSafeDurationModifiers; } private Trip doMap(org.onebusaway.gtfs.model.Trip rhs) { @@ -74,11 +74,11 @@ private Trip doMap(org.onebusaway.gtfs.model.Trip rhs) { lhs.withGtfsFareId(rhs.getFareId()); var trip = lhs.build(); - mapSafeDurationFactors(rhs).ifPresent(f -> flexSafeDurationFactors.put(trip, f)); + mapSafeDurationModifier(rhs).ifPresent(f -> flexSafeDurationModifiers.put(trip, f)); return trip; } - private Optional mapSafeDurationFactors(org.onebusaway.gtfs.model.Trip rhs) { + private Optional mapSafeDurationModifier(org.onebusaway.gtfs.model.Trip rhs) { if (rhs.getSafeDurationFactor() == null && rhs.getSafeDurationOffset() == null) { return Optional.empty(); } else { diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java index 4f0c70f22d2..141c1d5bf6b 100644 --- a/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java +++ b/src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java @@ -33,11 +33,15 @@ public class TripMapperTest { public static final DataImportIssueStore ISSUE_STORE = DataImportIssueStore.NOOP; - private final TripMapper subject = new TripMapper( - new RouteMapper(new AgencyMapper(FEED_ID), ISSUE_STORE, new TranslationHelper()), - new DirectionMapper(ISSUE_STORE), - new TranslationHelper() - ); + private final TripMapper subject = defaultTripMapper(); + + private static TripMapper defaultTripMapper() { + return new TripMapper( + new RouteMapper(new AgencyMapper(FEED_ID), ISSUE_STORE, new TranslationHelper()), + new DirectionMapper(ISSUE_STORE), + new TranslationHelper() + ); + } static { GtfsTestData data = new GtfsTestData(); @@ -56,14 +60,14 @@ public class TripMapperTest { } @Test - public void testMapCollection() throws Exception { + void testMapCollection() throws Exception { assertNull(subject.map((Collection) null)); assertTrue(subject.map(Collections.emptyList()).isEmpty()); assertEquals(1, subject.map(Collections.singleton(TRIP)).size()); } @Test - public void testMap() throws Exception { + void testMap() throws Exception { org.opentripplanner.transit.model.timetable.Trip result = subject.map(TRIP); assertEquals("A:1", result.getId().toString()); @@ -80,7 +84,7 @@ public void testMap() throws Exception { } @Test - public void testMapWithNulls() throws Exception { + void testMapWithNulls() throws Exception { Trip input = new Trip(); input.setId(AGENCY_AND_ID); input.setRoute(new GtfsTestData().route); @@ -101,12 +105,33 @@ public void testMapWithNulls() throws Exception { assertEquals(BikeAccess.UNKNOWN, result.getBikesAllowed()); } - /** Mapping the same object twice, should return the the same instance. */ + /** Mapping the same object twice, should return the same instance. */ @Test - public void testMapCache() throws Exception { + void testMapCache() throws Exception { org.opentripplanner.transit.model.timetable.Trip result1 = subject.map(TRIP); org.opentripplanner.transit.model.timetable.Trip result2 = subject.map(TRIP); assertSame(result1, result2); } + + @Test + void noFlexDurationModifier() { + var mapper = defaultTripMapper(); + mapper.map(TRIP); + assertTrue(mapper.flexSafeDurationModifiers().isEmpty()); + } + + @Test + void flexDurationModifier() { + 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.factor()); + assertEquals(600, mod.offsetInSeconds()); + } }