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

Clarify behavior for "schedule_relationship: SKIPPED" in StopTimeUpdates #139

Merged
merged 4 commits into from
Mar 18, 2019

Conversation

barbeau
Copy link
Collaborator

@barbeau barbeau commented Feb 7, 2019

Past discussions regarding StopTimeUpdate.ScheduleRelationship SKIPPED values revealed that the exact behavior of the consumer isn't well defined in regards to whether the SKIPPED value should be propagated to downstream stops, as well as whether delay values upstream of the SKIPPED stop should be propagated downstream of the SKIPPED stop.

We did a review of existing use of SKIPPED values in feeds, and followed up with a few agencies to clarify their intentions when publishing when this wasn't clear from the feed itself.

This proposal clarifies that SKIPPED values should not be propagated downstream, and that delay values should not be propagated downstream of the SKIPPED stop. EDIT Based on existing consumer behavior discussed below, delays will be propagated over the SKIPPED stop (changed in 9efe24e).

Announced on the GTFS-realtime Google Group at https://groups.google.com/forum/#!topic/gtfs-realtime/L07XRNypZrE.

EDIT:

Summary of exiting behavior

Consumer Propagate SKIPPED? Propagate delay?
OneBusAway No ?
Transit App Yes ?
OpenTripPlanner Unsupported Unsupported
TransitClock Unsupported Unsupported
Google No Yes

@gcamp
Copy link
Contributor

gcamp commented Feb 7, 2019

I wouldn't say this clarifies the behaviour, more that is changes it. My interpretation from the your research is that a 2 agencies use SKIPPED incorrectly.

The previous behaviour might not be obvious, but it wasn't ambiguous : there was always a propagation. But this seems even more confusing to me.

To me the fix should be to make the propagation behaviour more obvious in the spec, not to change the behaviour. I've seen agencies misuse the propagation behaviour of StopTimeUpdate.ScheduleRelationship.SCHEDULED too, but I think there's consensus to not change that behaviour.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 7, 2019

@gcamp Are you saying that currently the SKIPPED value is propagated down the trip, meaning that all stops downstream of a stop marked SKIPPED are also skipped if no other data is provided? Or that delays are currently propagated through SKIPPED stops?

@gcamp
Copy link
Contributor

gcamp commented Feb 8, 2019

@barbeau The SKIPPED value is propagated.

The current spec says

If one or more stops are missing along the trip the update is propagated to all subsequent stops. This means that updating a stop time for a certain stop will change all subsequent stops in the absence of any other information.

Any update is propagated, not only the delay, the entire update is. Doesn't that mean the SKIPPED ones would be too?

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 14, 2019

Any update is propagated, not only the delay, the entire update is. Doesn't that mean the SKIPPED ones would be too?

@gcamp I've never interpreted the spec this way, although given the text I can see how you could assume this (hence this PR to clarify things :)). I've read "update" referring to the delay within the STU, not the entire STU.

My rationale - there are fields in the StopTimeUpdate that obviously can't be propagated, like stop_id and stop_sequence, and other fields such as StopTimeEvent.time which need to be transformed to a delay before being propagated. uncertainty would naturally be propagated along with the predicted time/delay. Propagation rules for NO_DATA are explicitly called out for the NO_DATA field. SCHEDULED is the default value and I'm not sure how this could be abused (but a question on this below), so SKIPPED is the only field within the STU for which propagation behavior isn't defined at all.

Coincidentally, it looks like folks working on OneBusAway just implemented SKIPPED support without propagation (they were unaware of my proposal here).

And yes, I do plan to revisit improving the definition of arrival/departure time propagation previously discussed in #110.

Also, what do you mean by:

I've seen agencies misuse the propagation behaviour of StopTimeUpdate.ScheduleRelationship.SCHEDULED too, but I think there's consensus to not change that behaviour.

@gcamp
Copy link
Contributor

gcamp commented Feb 15, 2019

I've had to explain the propagation behaviour multiple times for agencies because they don't expect propagation at all. Even with StopTimeUpdate.ScheduleRelationship.SCHEDULED updates, I've seen agencies asking why they would see an update if it's not included in the GTFS-rt (they were missing aNO_DATA to stop propagation if that was their expectations).

It's quite possible I've misunderstood the propagation for SKIPPED, but doing it the proposed way seems to be against the initial rational to do propagation. My understanding was that propagation was implemented to reduce protobuf file size, but not allowing that capability for SKIPPED will force to have bigger protobuf files (as there are often multiple SKIPPED stops in a row).

I'm curious about other implementations of this that can way in here, the OTP example is a good one. If we are the only implementation that does it this way, we'll certainly change it.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 15, 2019

@abyrd, @sdjacobs, or @drewda Are you able to describe how OTP handles GTFS-rt SKIPPED values in StopTimeUpdates? Specifically, if there is an upstream SKIPPED value and downstream stops without any real-time info, is the SKIPPED value propagated downstream? In other words, are the following stops in the trip also assumed to be skipped?

@drewda
Copy link

drewda commented Feb 15, 2019

I'm not sure what OTP does.

If @TheTransitClock makes use of SKIPPED, perhaps @scrudden can offer another RT data consumer's perspective on this question?

@sdjacobs
Copy link

Unfortunately, OpenTripPlanner does not support SKIPPED stops. It'll simply discard TripUpdates that have StopTimeUpdates which have ScheduleRelationship = SKIPPED:

https://github.com/opentripplanner/OpenTripPlanner/blob/12d5d89aa95482e09e410152980a4b5b8633f594/src/main/java/org/opentripplanner/routing/edgetype/Timetable.java#L533-L536

@abyrd
Copy link

abyrd commented Feb 18, 2019

OpenTripPlanner GTFS-RT support was added in the context of the Plannerstack project in the Netherlands, and was built specifically to consume data in that context, where all data is being converted from local standards that do not align exactly with GTFS-RT. The details of how OTP handles GTFS-RT should not be interpreted as a position on how GTFS-RT should be interpreted, or used as a justification for any interpretation of the spec.

Personally, I would probably assume the behavior @barbeau is suggesting in this PR: that delay (a derived value) would be propagated, but not fields like SKIPPED. I don't think there's much point in trying to guess or reason about what the specification "really means", I think the specification is just ambiguous and needs to be clarified.

It's interesting that @gcamp says "I've had to explain the propagation behaviour multiple times for agencies because they don't expect propagation at all". I can definitely see the logic in not fabricating / extrapolating messages that were not sent by the producer. The Plannerstack project took this one step further and decided it would be better to refuse any message that didn't give a stop time update for every stop in a trip.

But it seems like propagation is expected and desired by some data producers, who are relying on systems that only provide data about current status of vehicles and expect the consumer to figure out what effects that might have. I guess this is reasonable considering the overall spirit of GTFS being easy to produce to encourage widespread adoption.

@scrudden
Copy link

@TheTransitClock consumes GTFS real-time vehicle positions to perform its main function. It does not consume trip updates. So, it does not make use of SKIPPED.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 18, 2019

So here's a summary of existing behavior so far:

Consumer Propagate SKIPPED?
OneBusAway No
Transit App Yes
OTP Unsupported
TransitClock Unsupported

I'm checking with Google to see what their current behavior is.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 19, 2019

Ok, Google behavior is that SKIPPED is not propagated, but delays are propagated over SKIPPED stops. I've proposed that delays aren't propagated, but I'm willing to change that if current consumer behavior is different (I believe OneBusAway matches Google behavior on delay propagation too, although I'm still confirming this).

I've updated a table in the first post with the current consumer behavior.

@gcamp Would Transit be willing to change to not propagating SKIPPED and propagating delays over the SKIPPED stop, to match Google's implementation (and I believe OBA)?

Any more consumers that want to weigh in with existing behavior?

@gcamp
Copy link
Contributor

gcamp commented Feb 20, 2019

Ok to change the behaviour on our side, I'm surprised nobody else interpreted the way we did however. But if Google did it this way, the sad truth is that it probably should be the reference implementation.

I would also change this part in trip_update

If one or more stops are missing along the trip the update is propagated to all subsequent stops. This means that updating a stop time for a certain stop will change all subsequent stops in the absence of any other information.

to

If one or more stops are missing along the trip the delay from that update is propagated to all subsequent stops. This means that updating a stop time for a certain stop will change all subsequent stops in the absence of any other information. Note that SKIPPED updates will not break a delay propagation, but SCHEDULED and NO_DATA will.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 20, 2019

@gcamp Good idea - I updated this in the Trip Updates docs in 34b21b9, which some slight wording changes. PTAL.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 22, 2019

In #144 @gcamp raised the question of whether a time only StopTimeUpdate should also be propagated (we previously referenced "delay" in this PR text, but didn't explicitly mention the case for when time is provided instead of delay). Transit App, Google, and OneBusAway all propagate time by computing a delay based on the comparison of the time and GTFS schedule time.

We're already touching that same text in this PR, so I've added further clarification in 8a0e003 that the propagation we're referring to also includes delays computed by comparing the provided time against the GTFS schedule.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 28, 2019

@gcamp Any further thoughts on this proposal? I'd like to open a vote on this in the next few days.

@gcamp
Copy link
Contributor

gcamp commented Feb 28, 2019

@barbeau all good for me

@barbeau
Copy link
Collaborator Author

barbeau commented Mar 1, 2019

This pull request has been open for more than one week, so per the Official Process I'm calling for a vote.

Vote will be closed on Friday March 8th at 23:59:59 UTC.

@gcamp
Copy link
Contributor

gcamp commented Mar 1, 2019

+1

@harringtonp
Copy link

+1

Had this discussion at

https://groups.google.com/forum/#!searchin/gtfs-realtime/skipped|sort:date/gtfs-realtime/teaI9X1FoGE/5-BIbl_qAgAJ

over 2 years ago and ended up changing the yourstop implementation to be exactly the same as that proposed above

@jakluk
Copy link

jakluk commented Mar 6, 2019

+1

@barbeau
Copy link
Collaborator Author

barbeau commented Mar 18, 2019

Voting is closed on this proposal and the results are:

Yes - 3

So it passes! Thanks all!

@barbeau barbeau merged commit 42bd689 into google:master Mar 18, 2019
@barbeau barbeau deleted the v26-skipped-clarification branch March 18, 2019 20:32
@laurentg
Copy link

I maybe a bit late on this, but our implementation of various AVL data to GTFS-RT in our Urbiplan tool currently do the exact same thing as this proposal: that is propagating only delays, and not SKIPPED / NO_DATA entries. If I had to vote, I would have voted +1 (not that this matter much anyway, given the overall majority so far).

@laurentg
Copy link

To be completely exact, our internal propagation rules do a linear interpolation of delays between existing data (SCHEDULED) to fill missing entries. Otherwise in some occasion time travel can occurs, and just clamping delays can also lead to illogical results. We though at a time about interpolating SKIPPED entries in the case a set of missing stop times were surrounded by 2 SKIPPED, but that idea was dropped.

@abyrd
Copy link

abyrd commented Mar 20, 2019

@laurentg said:

To be completely exact, our internal propagation rules do a linear interpolation of delays between existing data (SCHEDULED) to fill missing entries.

This is a very good point. The default propagation behavior of just replicating the delay value until the point in the trip where a new one is provided can create incoherent results (negative travel times between stops, i.e. arriving at a downstream stop earlier than you depart from the upstream stop).

This kind of complexities are the reason that we came to the conclusion that such propagation should ideally be handled by an external component with more sophisticated strategy and invariant checking. If simple propagation rules are a permanent part of the specification, they should be accompanied by some guidance on common pitfalls of this kind, to alert consumers of the additional checks and fixes they need to implement.

By the way, likewise I would have voted +1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GTFS Realtime Issues and Pull Requests that focus on GTFS Realtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants