-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adjust trip modifications #19
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments from Swiftly's review. Thanks!
The sequence of `replacement_stops` may be of arbitrary length. For example, 3 stops could be replaced by 2, 4, or 0 stops as the situation may require. | ||
The sequence of `replacement_stops` may be of arbitrary length. For example, 3 stops could be replaced by 2, 4, or 0 stops as the situation may require. | ||
|
||
It's allowed to replace a stop with a `ReplacementStop` that is the same as in the schedule if some other value are changing (like `dwell_time` `travel_time_to_stop`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate or give an example when you expect this might be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mainly to fix this issue : google#487, in which the described situation make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The common case might actually be the opposite, some dwell time might get reduced when reaching a time point with a planned detour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for linking the google issue. I had missed that one.
In the google issue example you linked, I was assuming stop "X" a detour temp stop - did you think the same? For detour temp-stops I see a benefit in being able to specify a desired dwell time as a mechanism for setting the temp stop departure time to something other than the arrival time.
In response to your last comment
The common case might actually be the opposite, some dwell time might get reduced when reaching a time point with a planned detour.
We generally assume the scheduled departure time is set for a timepoint and the dwell time is whatever it has to be for the bus to leave "on time" rather than assuming a bus will dwell for 5 the "scheduled dwell time" even if it arrives late. I think we would continue to assume this if the stop is configured as a timepoint.
I don't think TripModifications is the right standard for updating a regularly scheduled stops properties, particularly if the goal is to indicate that the bus will wait at the timepoint after the detour until the original scheduled time. If the stop is not a timepoint in static GTFS, I concede it could be useful to be able to say like "treat this stop as a timepoint now", but then I think "timepoint" should be the field, not dwell_time. But I would rather that go in the experimental stop_time_properties field that is within a TripUpdate's stop_time_update (if there is no real time info and this is the info update, stop_time_update.schedule_relationship could be specified as NO_DATA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a time_point value is enough for us if the dwell time is going to be reduced or not, or to which amount it's going to be reduced. I think agencies might have enough operational differences that we can't really assume anything there (but you might know more than me on that).
I'm ok dropping this for now, I don't think it's absolutely required, however I do think we'll need to support fully "replace existing stops by the same stop with different properties" and remove the assumption that all ReplacementStop are actually detour temp stops at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think separating this into a separate PR makes sense. Thanks for doing that. You do still have one mention of dwell in the reference.md file. Otherwise, looks good to me
- Addition of start_time and start_date to `ModifiedTripSelector` for frequency service support - Adding missing extensions fields for ModifiedTripSelector - Explicitly allow use of `schedule_relationship` in `TripDescriptor` when using a `ModifiedTripSelector`. - Add missing documentation in reference.md (it was only specified in the .proto)
b9bef5b
to
6761069
Compare
ModifiedTripSelector
for frequency service supportschedule_relationship
inTripDescriptor
when using aModifiedTripSelector
.