Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@
public class TimePenaltyCalculator implements FlexPathCalculator {
Comment thread
leonardehrenfried marked this conversation as resolved.

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
Expand All @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -45,7 +47,9 @@
* <p>
* For a discussion of this behaviour see https://github.com/MobilityData/gtfs-flex/issues/76
*/
public class UnscheduledTrip extends FlexTrip<UnscheduledTrip, UnscheduledTripBuilder> {
public class UnscheduledTrip
extends FlexTrip<UnscheduledTrip, UnscheduledTripBuilder>
implements LogInfo {

private static final Set<Integer> N_STOPS = Set.of(1, 2);
private static final int INDEX_NOT_FOUND = -1;
Expand Down Expand Up @@ -154,6 +158,12 @@ public Stream<FlexAccessTemplate> getFlexAccessTemplates(
);
}

@Nullable
@Override
public String logName() {
return "timePenalty=(%s)".formatted(timePenalty.toString());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is what the logName() method is intended for - it is to add name/description to entity logging. If the goal is to tests the penalty, the best way to do it is to test that the calculator retuned is of type TimePenaltyCalculator - that should be suffisient, since I assume there is unit tests on the calculator - no need to test the same features twice. Theoretical this is a white-box testing, but it is better than nothing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a bit stupid because I should have thought of that myself. 🤦‍♂️

}

/**
* Get the correct {@link FlexPathCalculator} depending on the {@code timePenalty}.
* If the modifier doesn't actually modify, we return the regular calculator.
Comment thread
leonardehrenfried marked this conversation as resolved.
Outdated
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
10 changes: 5 additions & 5 deletions src/main/java/org/opentripplanner/gtfs/mapping/TripMapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<>();

Expand Down Expand Up @@ -214,7 +214,7 @@ public TripStopTimes getStopTimesSortedByTrip() {
}

public Map<Trip, TimePenalty> getFlexTimePenalty() {
return flexDurationFactors;
return flexTimePenalties;
}

public EntityById<FareZone> getFareZonesById() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,22 +111,22 @@ 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);
var mod = mapper.flexSafeTimePenalties().get(mapped);
Comment thread
leonardehrenfried marked this conversation as resolved.
Outdated
assertEquals(1.5f, mod.coefficient());
assertEquals(600, mod.constant().toSeconds());
}
Expand Down