Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add via to the Transmodel trip query and make a proper Raptor implementation for it #6084

Open
wants to merge 42 commits into
base: dev-2.x
Choose a base branch
from

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Sep 22, 2024

Summary

This PR add back the via search to the Transmodel plan query. It does so by "chaining" Raptor requests. To be able to do this I had to refactor quite a bit. The functional changes is not that big.

This PR changes the Transmodel API and add a via location to the plan request. The passThrough feature works as before, but is merged with the new via API, and the old passThrough arguments are deprecated.

There are a few things left to implement:

  • support for coordinate via locations. This is implemented in Raptor, but there is no module test for it. To implement this "transfers" must be generated between the to Raptor searches. To find all transfer, find the paths for all stops nearby and put all pair of them together to form a transfer (stop A -> coordinate -> stop B).
  • Refactor Raptor passthrough to use the chaning feature. This will allow us to use the pass-through with transit-group-priority and also heuristics. To do this Raptor must support starting a journey on-board witch is useful in other use-cases as well. When this is done, we can remove the existing pass-through implementation.
  • Mixing visit-via and passthrough-via. There is two ways to do it, either by using the existing pass-through (messy setup) or doing the above first, and we will get it for free.
  • Enable the heuristics for pass-through.
  • Add support to GTFS API. @leonardehrenfried Will add this in this PR.
  • Add support to Debug UI
  • Remove relaxCostAtDestination
  • Add support for via in AStar (direct-street)
  • Add information in response for via? There is no tagging for via point in the path/itinerary. This can create problems in e.g. TransferOptimizer.
  • Preserve via results in OptimizedTransfer - this is an existing bug with pass-through as well.

I will create issues for reminding features, when this is merged.

The feature is a bit experimental and need "real life" testing - small changes and bug fixes are expected.

If this PR is too massive, then I can split it - but I believe there is little risk in breaking existing functionality. Splitting it in a pure feature PR and refactorings is difficult.

Issue

Related to : #4887

Unit tests

This PR refactor a lot of code and also add missing unit-test to some of the refactored classes. Almost all new code have unit-tests.

Documentation

✅ The API is documented and all new public classes/methods should have JavaDoc.

Changelog

Bumping the serialization version id

✅ The routing request is changed

@t2gran t2gran added New Feature bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Sep 22, 2024
@t2gran t2gran added this to the 2.7 (next release) milestone Sep 22, 2024
@t2gran t2gran requested a review from a team as a code owner September 22, 2024 19:06
Comment on lines +24 to +28
DATE_SCALAR = DateScalarFactory.createTransmodelDateScalar();
DOUBLE_FUNCTION_SCALAR = DoubleFunctionFactory.createDoubleFunctionScalar();
LOCAL_TIME_SCALAR = LocalTimeScalarFactory.createLocalTimeScalar();
TIME_SCALAR = TimeScalarFactory.createSecondsSinceMidnightAsTimeObject();
DURATION_SCALAR = DurationScalarFactory.createDurationScalar();
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason this is in a static initializer and not done during variable assignment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just readablility, the lines would break.

One of the listed stop locations must be visited on-board a transit vehicle or the journey must
alight or board at the location.
"""
input PlanPassThroughViaLocationInput {
Copy link
Member

Choose a reason for hiding this comment

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

Does the Transmodel API use the "plan" terminology?

the closest point in the street network from a stop and back to another stop to join
the transit network.

NOTE! Coordinates are NOT supported jet.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
NOTE! Coordinates are NOT supported jet.
NOTE! Coordinates are NOT supported yet.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that you want to allow clients to define both the coordinates and stopLocationIds in the same object with one being silently used as a fallback?

Comment on lines 18 to 19
* transit, or at the alight- or board-stop. The from-stop and to-stop is the same, and the
* minimum-wait-time must be zero.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the last sentence? Does it mean that you can only get off and then on again at the very same stop?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, to specify a passThroughStop from === to- only one visit to the stop is requiered - can be on-board. I will consider changing the design later.

import org.opentripplanner.raptor.api.model.RaptorTripSchedule;

/**
* Interface for Raptor Router. Allow instrumentation/wrapping the router. This is not
Copy link
Member

Choose a reason for hiding this comment

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

Is this still true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indead


private final ArrivalParetoSetComparatorFactory<McStopArrival<T>> comparatorFactory;
//private final ArrivalParetoSetComparatorFactory<McStopArrival<T>> comparatorFactory;
Copy link
Member

Choose a reason for hiding this comment

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

Can this line be deleted?

@@ -115,7 +129,9 @@ private RaptorRequest<T> doMap() {
var r = pt.raptor();

// Note! If a pass-through-point exists, then the transit-group-priority feature is disabled
if (!request.getPassThroughPoints().isEmpty()) {

// TODO - We need handle via locations that are not pass-through-points here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO - We need handle via locations that are not pass-through-points here
// TODO - We need to handle via locations that are not pass-through-points here

/**
* Return all stops associated with the given id. If a Station, a MultiModalStation, or a
* GroupOfStations matches the id, then all child stops are returned. If the id matches a regular
* stops, area stop or stop group, then a list with one item is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* stops, area stop or stop group, then a list with one item is returned.
* stop, area stop or stop group, then a list with one item is returned.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I've read through this PR and left comments.

My main feedback:

  • graphql-java has supported @OneOf for a while. does it not work for you?
  • I would like to know why the stopId/coordinate fallback strategy was chosen.

@leonardehrenfried
Copy link
Member

Also, you need to resolve some conflicts.

Comment on lines 18 to 19
* transit, or at the alight- or board-stop. The from-stop and to-stop is the same, and the
* minimum-wait-time must be zero.
Copy link
Member Author

Choose a reason for hiding this comment

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

No, to specify a passThroughStop from === to- only one visit to the stop is requiered - can be on-board. I will consider changing the design later.

@t2gran
Copy link
Member Author

t2gran commented Sep 26, 2024

@t2gran Do you expect this PR to change the performance?

No, chaning Raptor Request should only affect performance for via searches. There are minor changes in the Raptor instrumentation (not in logic), so it might cause some problems for the JIT compiler - but I expect that it should not change.

viaConnections
.byFromStop()
.forEachEntry((stop, connections) -> {
this.arrivals[stop] =
Copy link
Member Author

@t2gran t2gran Sep 30, 2024

Choose a reason for hiding this comment

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

This is OK, I though that the egress would overwrite this if the egress and vi was the same stop, but a McStopArrivals is 1-to-1 with the worker, so it has either via stops or egress, not both.

Comment on lines +263 to +264
egressPaths().egressesWitchStartByWalking(),
egressPaths().egressesWitchStartByARide()

Choose a reason for hiding this comment

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

An existing typo.

Suggested change
egressPaths().egressesWitchStartByWalking(),
egressPaths().egressesWitchStartByARide()
egressPaths().egressesWhichStartByWalking(),
egressPaths().egressesWhichStartByARide()

Comment on lines 30 to 31
/** private to prevent util class from instantiation */
public GqlUtil(ZoneId timeZone) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to anything you did, but this comment is wrong. This seems to be instantiated in a test.

Comment on lines +220 to 221
@Deprecated
public static Duration requireNonNegativeLong(Duration value, String subject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nice, but now these deprecated methods could delegate to the new generic method instead of having their own implementations right?

Comment on lines +14 to +15
* the connection to visit something other than a stop, like a street location. The result should be
* 100 connections. This is not an alternative to transfers. Raptor supports several use-cases
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence doesn't make sense to me, it probably belongs in the example further down.

Suggested change
* the connection to visit something other than a stop, like a street location. The result should be
* 100 connections. This is not an alternative to transfers. Raptor supports several use-cases
* the connection to visit something other than a stop, like a street location.
* This is not an alternative to transfers. Raptor supports several use-cases

* <h4>Route via a coordinate</h4>
*
* To route through a coordinate you need to find all nearby stops, then find all access and egress
* paths to and from the street location. The combine all access and egress paths to form
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* paths to and from the street location. The combine all access and egress paths to form
* paths to and from the street location. Then combine all access and egress paths to form

}

/**
* The egress path for search, if and only if this is the last leg.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The egress path for search, if and only if this is the last leg.
* The egress path for search. Non-null if and only if this is the last leg.


/**
* Set the time at a transit index iff it is optimal. This sets both the best time and the
* transfer time
*/
public McStopArrivals(
int nStops,
EgressPaths egressPaths,
AccessPaths accessPaths,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we're not using this accessPaths parameter anymore.

DestinationArrivalPaths<T> paths,
McStopArrivals<T> next,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could have a more descriptive name, like nextLegStopArrivals or something like that?

Comment on lines 182 to 183
EgressPaths egressPaths,
DestinationArrivalPaths<T> paths
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EgressPaths egressPaths,
DestinationArrivalPaths<T> paths
@Nullable EgressPaths egressPaths,
DestinationArrivalPaths<T> paths

@@ -20,4 +21,27 @@ McStopArrival<T> createTransferStopArrival(
RaptorTransfer transfer,
int arrivalTime
);

default McStopArrival<T> createViaStopArrival(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default McStopArrival<T> createViaStopArrival(
@Nullable
default McStopArrival<T> createViaStopArrival(

Copy link
Member

@optionsome optionsome left a comment

Choose a reason for hiding this comment

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

This is purely a code review and I didn't even review the tests (yet).

}
);
callWith.argument(
TripQuery.FIELD_VIA,
Copy link
Member

Choose a reason for hiding this comment

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

I think the root parameters of a query are called parameters. GraphQL's official website seems to call the fields inside input types fields.

Comment on lines +9 to +11
* Validate @oneOf directive, this validation is NOT done by the Java GraphQL library at the
* moment(v22.1). Remove this when enforced by the library. The {@code @oneOf} is an experimental
* feature in this version of the library.
Copy link
Member

Choose a reason for hiding this comment

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

Could probably specify here that the GraphQL java doesn't do the validation when the API is implemented like way the Transmodel API is implemented through code-first approach.

import org.opentripplanner.routing.api.response.RoutingError;
import org.opentripplanner.routing.api.response.RoutingErrorCode;
import org.opentripplanner.routing.error.RoutingValidationException;

class ViaLocationMapper {
@Deprecated
class ViaLocationDeprecatedMapper {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably figure out some unified naming for these types of classes. I think I've called them LegacySomething in the past and in this pr, sometimes the legacy word is used to refer to the old implementation.

Comment on lines +8 to +13
public static final GraphQLDirective TIMING_DATA = GraphQLDirective
.newDirective()
.name("timingData")
.description("Add timing data to prometheus, if Actuator API is enabled")
.validLocation(Introspection.DirectiveLocation.FIELD_DEFINITION)
.build();
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't implemented within the scope of this pr but it feels weird to expose this type of information through the schema to API users or do we somehow remove this from the introspection response?

Comment on lines +24 to +28
A visit-via-location is a physical visit to one of the stops or coordinates listed. An
on-board visit does not count, the traveler must alight or board at the given stop for
it to to be accepted. To visit a coordinate, the traveler must walk(bike or drive) to
the closest point in the street network from a stop and back to another stop to join
the transit network.
Copy link
Member

Choose a reason for hiding this comment

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

I remember we discussed how the visit stops should work in some previous developer meeting and I remember there were some differing opinions around it. I do see the benefit of being able to specify a boarding/alighting stop but I also would see benefit in being able to specify a station as a via location without actually needing to connect to the transit network at said station, similarly as we currently support for origin and destination. Although, we don't support direct street via searches so it gives us more options.

Comment on lines +2101 to +2102
One of the listed stop locations must be visited on-board a transit vehicle or the journey must
alight or board at the location.
Copy link
Member

Choose a reason for hiding this comment

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

Was it possible with pass through points that you walk from the origin to a passthrough stop and board from there (not just from walking from a transfer stop)?

Comment on lines +2115 to +2121
"""
A via-location is used to specifying a location as an intermediate place the router must
route through. The via-location must be either a pass-through-location or a
visit-via-location. An on-board "visit" is only allowed for pass-through-via-locations, while
the visit-via-location can visit a stop-location or a coordinate and specify a
minimum-wait-time.
"""
Copy link
Member

Choose a reason for hiding this comment

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

"An on-board "visit" is only allowed for pass-through-via-locations" this can be interpreted in two ways, either it limits the passthrough point or a visit point. The difference between the two types is not completely clear as it's not clear what visit means and sometimes passthrough point and visit stop location both act similarly as a boarding/alighting location.

Comment on lines +2123 to +2124
passThrough: PlanPassThroughViaLocationInput
visit: PlanVisitViaLocationInput
Copy link
Member

Choose a reason for hiding this comment

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

These are missing descriptions. I think we usually put descriptions both on the field and on the type.

Comment on lines +2130 to +2132
it to to be accepted. To visit a coordinate, the traveler must walk(bike or drive) to
the closest point in the street network from a stop and back to another stop to join
the transit network.
Copy link
Member

Choose a reason for hiding this comment

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

I guess here it could be explicitly specified that walking from origin to a coordinate or from a coordinate to destination is not allowed, even though you sort of say it here.

Comment on lines +2145 to +2147
A list of stop locations. A stop location can be a quay, a stop place, a multimodal
stop place or a group of stop places. It is enough to visit ONE of the locations
listed.
Copy link
Member

Choose a reason for hiding this comment

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

So once we add coordinates, it's enough that either one of the stop locations or coordinates is visited if both are defined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants