Skip to content

Cancel individual stop on StopPattern instead of TripTimes - #3575

Merged
gmellemstrand merged 13 commits into
opentripplanner:dev-2.xfrom
entur:otp2_stoptimes_cancellations
Aug 12, 2021
Merged

Cancel individual stop on StopPattern instead of TripTimes#3575
gmellemstrand merged 13 commits into
opentripplanner:dev-2.xfrom
entur:otp2_stoptimes_cancellations

Conversation

@gmellemstrand

@gmellemstrand gmellemstrand commented Jul 29, 2021

Copy link
Copy Markdown
Contributor

Summary

This build on top of #3571.

Because Raptor only checks whether stops can be boarded/alighted on the TripPattern level, we need to do the actual cancellations in the StopPattern instead of the TripTimes.

This PR does the following:

  • Change the Stop field on TripTimeOnDate to TripPattern. This allows checks for cancellations on the Pattern level.
  • Change the SIRI implementation to cancel stops at the StopPattern level. This will make the Siri implementation create a new TripPattern for that Trip.
  • Mark in the GTFS-RT implementation where cancelations at the TripPattern level is missing.
  • Simplify the Transmodel API calls for forBoarding and forAlighting by replacing the more complex logic with a call to PickUpType.isRouteable().
  • Add back the cancellation flags on the TripTimes class. These are only for API-purposes and do not affect routing. This is documented with javadoc. The only reason these exist is that the current realtime model does not allow navigating to the updated version of the TripTimes object. This is duplicated information.

Issue

#3119

Unit tests

No new unit tests

Changelog

Changelog added

@gmellemstrand
gmellemstrand marked this pull request as ready for review August 3, 2021 11:51
@gmellemstrand
gmellemstrand requested a review from a team as a code owner August 3, 2021 11:51
@gmellemstrand

Copy link
Copy Markdown
Contributor Author

A question @abyrd raised was whether we should change the names of the following fields on TripTimes to something that is more correct in English.

  • recordedStops
  • predictionInaccurateOnStops

@abyrd

abyrd commented Aug 4, 2021

Copy link
Copy Markdown
Member

A question @abyrd raised was whether we should change the names of the following fields on TripTimes to something that is more correct in English.

  • recordedStops
  • predictionInaccurateOnStops

To clarify - it's not so much that they're incorrect, more that because they are unconventional ways of saying something, they can confuse or create doubt for a native English speaker. These terms may be standardized terms from a specification, in which case we may want to keep them but just add documentation. Otherwise we may want to change the variable names. In either case they will require a little documentation.

The essence of recordedStops seems to be that they have been passed, or are in the past, or are somehow "completed".

The actual meaning of predictionInaccurate seems to be low-precision (i.e. the true value may deviate from the given expected value) rather than inaccurate which sounds like they are known to be wrong, something like "it's definitely not this value I'm giving you". Seeing "inaccurate" I initially wondered if this was saying a past prediction was now known to be wrong.

@abyrd abyrd left a comment

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.

We have discussed this several times and gone into details on the realtime handling, so I think this is ready for merge. I'm just requesting a couple of additions to Javadoc to make it easier to understand in the future. We can write this documentation together at today's meeting to ensure it's correct.

I believe this PR also leaves some cancellation logic unspecified - this should also be stated in the PR description.

* This is stop-specific, so the index i is a stop index, not a hop index.
*/
public TripTimeOnDate(TripTimes tripTimes, int stopIndex, Stop stop, ServiceDay serviceDay) {
public TripTimeOnDate(TripTimes tripTimes, int stopIndex, TripPattern tripPattern, ServiceDay serviceDay) {

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.

This could use a little more documentation. At first I didn't understand "stop-specific" and thought the name of this class should be plural: TripTimesOnDate. A "tripTime" could be understood to be a single row of GTFS stop_times.txt associated with a trip; the plural TripTimes is supposed to mean the sequence of stop_times for a single trip. Here we are indeed referencing a single trip time, not the whole sequence. The reference to TripPattern seems surprisingly "heavy" for something representing a single time, but if I understand correctly this is a throwaway object used in API translation. Putting all this info in Javadoc makes it easier to understand.

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.

Looking at this again I see that some of the info is already in the class javadoc, and this comment is on the constructor javadoc. It's also obsolete in that it mentions index i, but the parameter name is now clearly stopIndex. We decided the javadoc on this constructor is just not necessary.

*
* Non-final to allow updates.
*/
private boolean[] cancelledStops;

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 think this field was the root of our recent realtime discussions. Canceling a stop changes the pattern of visited stops (StopPattern), and each TripTimes is within a single TripPattern (via a Timetable), with a single StopPattern. The trip must then be moved to a different pattern. But when displaying the original unmodified (non-realtime) data (e.g. on a sign board) we still want to reveal that a stop has been canceled. This needs to be clarified somewhat in this Javadoc.

abyrd
abyrd previously approved these changes Aug 12, 2021

@abyrd abyrd left a comment

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.

Javadoc and comments have been updated. I see there's now a clear TODO about how boolean[] cancelledStops works and how it could be revised.

@gmellemstrand
gmellemstrand merged commit 646c6be into opentripplanner:dev-2.x Aug 12, 2021
@gmellemstrand
gmellemstrand deleted the otp2_stoptimes_cancellations branch August 12, 2021 11:01
@t2gran t2gran added this to the 2.1 milestone Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants