Skip to content

Commit

Permalink
Merge pull request #6255 from opentripplanner/transit-group-priority-…
Browse files Browse the repository at this point in the history 
…and-via-bug

Fix problem with relaxed-generalized-cost-at-destination
  • Loading branch information
@t2gran
t2gran authored Nov 22, 2024
2 parents 0202d1e + bdb6e54 commit 805e8d6
Show file tree
Hide file tree
Showing 5 changed files with 212 additions and 36 deletions.
31 changes: 21 additions & 10 deletions .../opentripplanner/routing/algorithm/raptoradapter/transit/mappers/RaptorRequestMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.opentripplanner.raptor.api.model.RaptorTripSchedule;
import org.opentripplanner.raptor.api.model.RelaxFunction;
import org.opentripplanner.raptor.api.request.DebugRequestBuilder;
import org.opentripplanner.raptor.api.request.MultiCriteriaRequest;
import org.opentripplanner.raptor.api.request.Optimization;
import org.opentripplanner.raptor.api.request.PassThroughPoint;
import org.opentripplanner.raptor.api.request.RaptorRequest;
Expand All @@ -26,6 +27,7 @@
import org.opentripplanner.routing.api.request.DebugEventType;
import org.opentripplanner.routing.api.request.RouteRequest;
import org.opentripplanner.routing.api.request.framework.CostLinearFunction;
import org.opentripplanner.routing.api.request.preference.TransitPreferences;
import org.opentripplanner.routing.api.request.via.ViaLocation;
import org.opentripplanner.transit.model.network.grouppriority.DefaultTransitGroupPriorityCalculator;

Expand Down Expand Up @@ -128,15 +130,20 @@ private RaptorRequest<T> doMap() {
var pt = preferences.transit();
var r = pt.raptor();

// Note! If a pass-through-point exists, then the transit-group-priority feature is disabled

// TODO - We need to handle via locations that are not pass-through-points here
if (hasPassThroughOnly()) {
mcBuilder.withPassThroughPoints(mapPassThroughPoints());
} else if (hasViaLocationsOnly()) {
builder.searchParams().addViaLocations(mapViaLocations());
// relax transit group priority can be used with via-visit-stop, but not with pass-through
if (pt.isRelaxTransitGroupPrioritySet()) {
mapRelaxTransitGroupPriority(mcBuilder, pt);
}
} else if (pt.isRelaxTransitGroupPrioritySet()) {
mapRelaxTransitGroupPriority(mcBuilder, pt);
} else {
// The deprecated relaxGeneralizedCostAtDestination is only enabled, if there is no
// via location and the relaxTransitGroupPriority is not used (Normal).
r.relaxGeneralizedCostAtDestination().ifPresent(mcBuilder::withRelaxCostAtDestination);
} else if (!pt.relaxTransitGroupPriority().isNormal()) {
mcBuilder.withTransitPriorityCalculator(new DefaultTransitGroupPriorityCalculator());
mcBuilder.withRelaxC1(mapRelaxCost(pt.relaxTransitGroupPriority()));
}
});

Expand All @@ -160,10 +167,6 @@ private RaptorRequest<T> doMap() {
.addAccessPaths(accessPaths)
.addEgressPaths(egressPaths);

if (hasViaLocationsOnly()) {
builder.searchParams().addViaLocations(mapViaLocations());
}

var raptorDebugging = request.journey().transit().raptorDebugging();

if (raptorDebugging.isEnabled()) {
Expand Down Expand Up @@ -271,6 +274,14 @@ private int relativeTime(Instant time) {
return (int) (time.getEpochSecond() - transitSearchTimeZeroEpocSecond);
}

private static void mapRelaxTransitGroupPriority(
MultiCriteriaRequest.Builder<?> mcBuilder,
TransitPreferences pt
) {
mcBuilder.withTransitPriorityCalculator(new DefaultTransitGroupPriorityCalculator());
mcBuilder.withRelaxC1(mapRelaxCost(pt.relaxTransitGroupPriority()));
}

private static void addLogListenerForEachEventTypeRequested(
DebugRequestBuilder target,
DebugEventType type,
Expand Down
3 changes: 2 additions & 1 deletion application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,9 @@ public List<ViaLocation> getViaLocations() {
return via;
}

public void setViaLocations(final List<ViaLocation> via) {
public RouteRequest setViaLocations(final List<ViaLocation> via) {
this.via = via;
return this;
}

/**
Expand Down
4 changes: 4 additions & 0 deletions .../src/main/java/org/opentripplanner/routing/api/request/preference/TransitPreferences.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ public CostLinearFunction relaxTransitGroupPriority() {
return relaxTransitGroupPriority;
}

public boolean isRelaxTransitGroupPrioritySet() {
return !relaxTransitGroupPriority.isNormal();
}

/**
* When true, real-time updates are ignored during this search.
*/
Expand Down
204 changes: 179 additions & 25 deletions ...ntripplanner/routing/algorithm/raptoradapter/transit/mappers/RaptorRequestMapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.RaptorRequestMapperTest.RequestFeature.RELAX_COST_DEST;
import static org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.RaptorRequestMapperTest.RequestFeature.TRANSIT_GROUP_PRIORITY;
import static org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.RaptorRequestMapperTest.RequestFeature.VIA_PASS_THROUGH;
import static org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.RaptorRequestMapperTest.RequestFeature.VIA_VISIT;

import java.time.Duration;
import java.time.ZonedDateTime;
import java.util.List;
import java.util.Map;
import java.util.stream.IntStream;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand All @@ -24,11 +30,22 @@
import org.opentripplanner.transit.model._data.TimetableRepositoryForTest;
import org.opentripplanner.transit.model.framework.FeedScopedId;
import org.opentripplanner.transit.model.site.StopLocation;
import org.opentripplanner.utils.collection.ListUtils;

class RaptorRequestMapperTest {

private static final TimetableRepositoryForTest TEST_MODEL = TimetableRepositoryForTest.of();
private static final StopLocation STOP_A = TEST_MODEL.stop("Stop:A").build();
public static final PassThroughViaLocation PASS_THROUGH_VIA_LOCATION = new PassThroughViaLocation(
"Via A",
List.of(STOP_A.getId())
);
public static final VisitViaLocation VISIT_VIA_LOCATION = new VisitViaLocation(
"Via A",
null,
List.of(STOP_A.getId()),
List.of()
);
private static final List<RaptorAccessEgress> ACCESS = List.of(TestAccessEgress.walk(12, 45));
private static final List<RaptorAccessEgress> EGRESS = List.of(TestAccessEgress.walk(144, 54));

Expand All @@ -37,6 +54,10 @@ class RaptorRequestMapperTest {
private static final CostLinearFunction R3 = CostLinearFunction.of("30 + 2.0x");

private static final Map<FeedScopedId, StopLocation> STOPS_MAP = Map.of(STOP_A.getId(), STOP_A);
private static final CostLinearFunction RELAX_TRANSIT_GROUP_PRIORITY = CostLinearFunction.of(
"30m + 1.2t"
);
public static final double RELAX_GENERALIZED_COST_AT_DESTINATION = 2.0;

static List<Arguments> testCasesRelaxedCost() {
return List.of(
Expand Down Expand Up @@ -102,40 +123,104 @@ void testTransitGroupPriority() {

var result = map(req);

assertFalse(result.multiCriteria().transitPriorityCalculator().isEmpty());
assertTrue(result.multiCriteria().transitPriorityCalculator().isPresent());
}

@Test
void testVisitViaAllowsTransitGroupPriority() {
var req = new RouteRequest();

// Set visit-via and relax transit-group-priority
req.setViaLocations(
List.of(new VisitViaLocation("Via A", null, List.of(STOP_A.getId()), List.of()))
);
req.withPreferences(p ->
p.withTransit(t -> t.withRelaxTransitGroupPriority(CostLinearFunction.of("30m + 1.2t")))
static List<Arguments> testViaAndTransitGroupPriorityCombinationsTestCases() {
return List.of(
// If ONE feature is requested, the same feature is expected
Arguments.of(
"VIA_PASS_THROUGH only",
List.of(VIA_PASS_THROUGH),
List.of(VIA_PASS_THROUGH),
null
),
Arguments.of("VIA_VISIT only", List.of(VIA_VISIT), List.of(VIA_VISIT), null),
Arguments.of(
"TRANSIT_GROUP_PRIORITY only",
List.of(TRANSIT_GROUP_PRIORITY),
List.of(TRANSIT_GROUP_PRIORITY),
null
),
Arguments.of(
"RELAX_COST_DEST only",
List.of(RELAX_COST_DEST),
List.of(RELAX_COST_DEST),
null
),
Arguments.of(
"VIA_VISIT is not allowed together VIA_PASS_THROUGH, an error is expected.",
List.of(VIA_VISIT, VIA_PASS_THROUGH),
List.of(),
"A mix of via-locations and pass-through is not allowed in this version."
),
Arguments.of(
"""
VIA_VISIT is not allowed together VIA_PASS_THROUGH, an error is expected.
Other features are ignored.
""",
List.of(VIA_VISIT, VIA_PASS_THROUGH, TRANSIT_GROUP_PRIORITY, RELAX_COST_DEST),
List.of(),
"A mix of via-locations and pass-through is not allowed in this version."
),
Arguments.of(
"VIA_PASS_THROUGH cannot be combined with other features, and other features are dropped",
List.of(VIA_PASS_THROUGH, TRANSIT_GROUP_PRIORITY, RELAX_COST_DEST),
List.of(VIA_PASS_THROUGH),
null
),
Arguments.of(
"VIA_VISIT can be combined with TRANSIT_GROUP_PRIORITY",
List.of(VIA_VISIT, TRANSIT_GROUP_PRIORITY),
List.of(VIA_VISIT, TRANSIT_GROUP_PRIORITY),
null
),
Arguments.of(
"""
VIA_VISIT can only be combined with TRANSIT_GROUP_PRIORITY, and other features are dropped
VIA_PASS_THROUGH override VIA_VISIT (see above)
""",
List.of(VIA_VISIT, TRANSIT_GROUP_PRIORITY, RELAX_COST_DEST),
List.of(VIA_VISIT, TRANSIT_GROUP_PRIORITY),
null
),
Arguments.of(
"""
TRANSIT_GROUP_PRIORITY cannot be combined with other features, override RELAX_COST_DEST
VIA_PASS_THROUGH and VIA_VISIT override VIA_VISIT (see above)
""",
List.of(TRANSIT_GROUP_PRIORITY, RELAX_COST_DEST),
List.of(TRANSIT_GROUP_PRIORITY),
null
)
);

var result = map(req);

assertFalse(result.multiCriteria().transitPriorityCalculator().isEmpty());
}

@Test
void testPassThroughPointsTurnTransitGroupPriorityOff() {
@ParameterizedTest(name = "{0}. {1} => {2}")
@MethodSource("testViaAndTransitGroupPriorityCombinationsTestCases")
void testViaAndTransitGroupPriorityCombinations(
String testDescription,
List<RequestFeature> requestedFeatures,
List<RequestFeature> expectedFeatures,
@Nullable String errorMessage
) {
var req = new RouteRequest();

// Set pass-through and relax transit-group-priority
req.setViaLocations(List.of(new PassThroughViaLocation("Via A", List.of(STOP_A.getId()))));
req.withPreferences(p ->
p.withTransit(t -> t.withRelaxTransitGroupPriority(CostLinearFunction.of("30m + 1.2t")))
);
for (RequestFeature it : requestedFeatures) {
req = setFeaturesOnRequest(req, it);
}

var result = map(req);
if (errorMessage == null) {
var result = map(req);

// transit-group-priority CANNOT be used with pass-through and is turned off...
assertTrue(result.multiCriteria().transitPriorityCalculator().isEmpty());
for (var feature : RequestFeature.values()) {
assertFeatureSet(feature, result, expectedFeatures.contains(feature));
}
} else {
var r = req;
var ex = Assertions.assertThrows(IllegalArgumentException.class, () -> map(r));
assertEquals(errorMessage, ex.getMessage());
}
}

private static RaptorRequest<TestTripSchedule> map(RouteRequest request) {
Expand All @@ -149,4 +234,73 @@ private static RaptorRequest<TestTripSchedule> map(RouteRequest request) {
id -> IntStream.of(STOPS_MAP.get(id).getIndex())
);
}

private static void assertFeatureSet(
RequestFeature feature,
RaptorRequest<?> result,
boolean expected
) {
switch (feature) {
case VIA_VISIT:
if (expected) {
assertTrue(result.searchParams().hasViaLocations());
// One via location exist(no NPE), but it does not allow pass-through
assertEquals(
"Via{label: Via A, connections: [0]}",
result.searchParams().viaLocations().get(0).toString()
);
}
break;
case VIA_PASS_THROUGH:
if (expected) {
assertTrue(result.multiCriteria().hasPassThroughPoints());
assertEquals(
"(Via A, stops: 0)",
result.multiCriteria().passThroughPoints().get(0).toString()
);
}
break;
case TRANSIT_GROUP_PRIORITY:
assertEquals(expected, result.multiCriteria().transitPriorityCalculator().isPresent());
if (expected) {
assertFalse(result.multiCriteria().hasPassThroughPoints());
}
break;
case RELAX_COST_DEST:
assertEquals(expected, result.multiCriteria().relaxCostAtDestination() != null);
if (expected) {
assertFalse(result.multiCriteria().hasPassThroughPoints());
assertFalse(result.searchParams().hasViaLocations());
}
break;
}
}

private static RouteRequest setFeaturesOnRequest(RouteRequest req, RequestFeature feature) {
return switch (feature) {
case VIA_VISIT -> req.setViaLocations(
ListUtils.combine(req.getViaLocations(), List.of(VISIT_VIA_LOCATION))
);
case VIA_PASS_THROUGH -> req.setViaLocations(
ListUtils.combine(req.getViaLocations(), List.of(PASS_THROUGH_VIA_LOCATION))
);
case TRANSIT_GROUP_PRIORITY -> req.withPreferences(p ->
p.withTransit(t -> t.withRelaxTransitGroupPriority(RELAX_TRANSIT_GROUP_PRIORITY))
);
case RELAX_COST_DEST -> req.withPreferences(p ->
p.withTransit(t ->
t.withRaptor(r ->
r.withRelaxGeneralizedCostAtDestination(RELAX_GENERALIZED_COST_AT_DESTINATION)
)
)
);
};
}

enum RequestFeature {
VIA_VISIT,
VIA_PASS_THROUGH,
TRANSIT_GROUP_PRIORITY,
RELAX_COST_DEST,
}
}
6 changes: 6 additions & 0 deletions .../test/java/org/opentripplanner/routing/api/request/preference/TransitPreferencesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ void relaxTransitGroupPriority() {
assertEquals(TRANSIT_GROUP_PRIORITY_RELAX, subject.relaxTransitGroupPriority());
}

@Test
void isRelaxTransitGroupPrioritySet() {
assertTrue(subject.isRelaxTransitGroupPrioritySet());
assertFalse(TransitPreferences.DEFAULT.isRelaxTransitGroupPrioritySet());
}

@Test
void ignoreRealtimeUpdates() {
assertFalse(TransitPreferences.DEFAULT.ignoreRealtimeUpdates());
Expand Down

0 comments on commit 805e8d6

Please sign in to comment.