NeTEx mapping to StopTime.timepoint by TimMickelsonSkanetrafiken · Pull Request #3898 · opentripplanner/OpenTripPlanner

TimMickelsonSkanetrafiken · 2022-02-11T08:59:59Z

Summary

As GTFS has a mapping stop_times.timepoint to OpenTripPlanner internal model StopTime.timepoint a similar timepoint exists in NeTEx. The short definition is that if the StopPointInJourneyPattern.isWaitPoint is true and the TimetabledPassingTime.WaitingTime is defined then the StopTime.timepoint is considered true (The vehicle has a planned waiting time on given stop).

The more complex description is that in NeTEx we are actually considering an object called TimingPoint. This object is more complex than an GTFS stop_times.timepoint that is basically a boolean. But a special case when the TimingPoint has set a WaitTime then the NeTEx TimingPoint is considered to have planned wait time if the vehicle is ahead of schedule. The TimingPoint in NeTEx can be linked to the JourneyPattern and if so the stop is a TimingPoint for all trips on this pattern. Actually there is a convinience boolean already included in NeTEx for if the WaitTime is set on the journey pattern, the already mentioned StopPointInJourneyPattern.isWaitPoint . The TimetabledPassingTime has no such boolean but could contain a TimingPoint reference to link the TimingPoint not only to the journey pattern but actually to the stop and the given trip. But thankfully, the TimetabledPassingTime has a convinience variable called WaitingTime, documented as Planned waiting time at a Point.

Note: Sadly the Transmodel GraphQl API exposes a boolean named timepoint, note that this name is not transmodel, it would probably be more correct with the name WaitPoint as in the StopPointInJourneyPattern.isWaitPoint. Even worse, then name timepoint is very easily confused with the existing Transmodel object TimingPoint. I don't wont to touch the name though, becuase it would be a breaking change.

Note2: The actual documentation on the exposed timepoint is Whether this is a timing point or not. Boarding and alighting is not allowed at timing points. This text does not seem to be true, and is not what is documented actually in GTFS (https://developers.google.com/transit/gtfs/reference#stop_timestxt). I would like to change this documentation, e.g. Indicates whether the stop has planned wait time for given trip, i.e. if the stop is a timing point.. Please let me know if you agree?

Unit tests

A test was added for the NeTEx mapping to check some different cases.

Documentation

GraphQl attribute is documented

hannesj · 2022-02-11T11:56:48Z

I think the timepoint description is wrong in the Transmodel API, an it should be used for this instead. It comes from the GTFS specification as follows:

Indicates if arrival and departure times for a stop are strictly adhered to by the vehicle or if they are instead approximate and/or interpolated times. This field allows a GTFS producer to provide interpolated stop-times, while indicating that the times are approximate.

leonardehrenfried · 2022-02-11T12:05:44Z

@hannesj Are you saying that GTFS timepoints and Netex waitpoints are the same thing?

TimMickelsonSkanetrafiken · 2022-02-11T12:40:23Z

I don't think that timepoint is the same as waitPoint, this is not how I interpret it. Please let me know if I'm wrong and we should use the field for bouth. We have a mix of GTFS and NeTEx so it would be important that they represent the same situation.

I can agree with that the documentation in GraphQl timepoint is different from GTFS documentation, but I don't know if it's part of this pull request to change and if so to what, the pull request regards first and most NeTEx.

hannesj · 2022-02-11T13:59:37Z

The definition for a time point, is that a vehicle will not leave it before the assigned departure time, even if it was running ahead of schedule before it. I think this is quite the same as a waitPoint in Netex, or what do you think is the difference?

https://en.wikipedia.org/wiki/Timing_point

barbeau · 2022-02-11T16:01:08Z

What's the NeTEx definition of waitPoint?

barbeau · 2022-02-11T16:03:16Z

-                    .description("Whether this is a timing point or not. Boarding and alighting is not allowed at timing points.")
-                    .dataFetcher(environment -> ((TripTimeOnDate) environment.getSource()).isTimepoint())
+                    .description(
+                            "Whether this is a timing point or not. Boarding and alighting is not allowed at timing points.")


In GTFS, you can board and alight at timepoints. Furthermore, in the current GTFS spec timepoints are always stops (i.e., timepoint is an attribute of a record in stops.txt).

We have talked about this PR internally, I will open an new and put a link here. The business logic for the NeTEx waitPoint is not OK and @hannesj you are right, the attribute exists already, the name should be different because it is not transmodel, but I will keep it as is. @barbeau I will explain in detail the NeTEx definition of waitPoint in the next PR. It will be much cleaner and simpler.

I’d suggest to just reuse this PR and force-push a new more clean commit. That way we can have the discussion and history in one place

@barbeau @hannesj I have updated this PR and simplified it much. Please look at my notes in the description if you have any opinion. Specially the description of timepoint in GraphQl actually seems wrong to me.

- Map from NeTEx to StopTime in StopTimesMapper - Add Unit test in StopTimesMapperTest for all permutations of mapping possibilties

barbeau · 2022-02-15T16:32:31Z

Note2: The actual documentation on the exposed timepoint is Whether this is a timing point or not. Boarding and alighting is not allowed at timing points. This text does not seem to be true, and is not what is documented actually in GTFS (https://developers.google.com/transit/gtfs/reference#stop_timestxt). I would like to change this documentation, e.g. Indicates whether the stop has planned wait time for given trip, i.e. if the stop is a timing point.. Please let me know if you agree?

I can't comment on the current behavior in OTP as I'm not familiar with the code, but I agree that the description does not match the GTFS definition of timepoint. Boarding and alighting is certainly allowed at timepoints in GTFS by default.

barbeau · 2022-02-15T16:38:13Z

As GTFS has a mapping stop_times.timepoint to OpenTripPlanner internal model StopTime.timepoint a similar timepoint exists in NeTEx. The short definition is that if the StopPointInJourneyPattern.isWaitPoint is true and the TimetabledPassingTime.WaitingTime is defined then the StopTime.timepoint is considered true (The vehicle has a planned waiting time on given stop).

Just to clarify the previous discussion - so it seems to me that conceptually the basic definition of StopPointInJourneyPattern.isWaitPoint in NeTEx and stopTime.timepoint in GTFS are the same - if the variable is set to true, and the vehicle is running ahead of schedule, it's expected to wait at the stop until the scheduled departure time.

@TimMickelsonSkanetrafiken Do you agree?

TimMickelsonSkanetrafiken · 2022-02-16T06:03:32Z

As GTFS has a mapping stop_times.timepoint to OpenTripPlanner internal model StopTime.timepoint a similar timepoint exists in NeTEx. The short definition is that if the StopPointInJourneyPattern.isWaitPoint is true and the TimetabledPassingTime.WaitingTime is defined then the StopTime.timepoint is considered true (The vehicle has a planned waiting time on given stop).

Just to clarify the previous discussion - so it seems to me that conceptually the basic definition of StopPointInJourneyPattern.isWaitPoint in NeTEx and stopTime.timepoint in GTFS are the same - if the variable is set to true, and the vehicle is running ahead of schedule, it's expected to wait at the stop until the scheduled departure time.

@TimMickelsonSkanetrafiken Do you agree?

@barbeau This is exactly what I mean, I just wanted to be very precise in my documentation. In NeTEx we have the TimingPoint and it is more complex than a GTFS time point, but when the NeTEx WaitingTime is defined, the NeTEx and GTFS data reflect the exact same situation that the vehicle is expected (scheduled) to wait if running ahead of time on this stop. The documentation in GraphQl should therefore be exactly the same indipendent of origin of data (GTFS or NeTEx).

t2gran · 2022-02-22T11:40:42Z

+            // From NeTEx we define timepoint as a waitpoint with waiting time defined (also 0)
+            if (Boolean.TRUE.equals(stopPoint.isIsWaitPoint())
+                    && passingTime.getWaitingTime() != null) {
+                stopTime.setTimepoint(1);


I think it would be an immprovment if the OTP internal model for time point would be an enum. The value 1 here is not telling me what this is. Then there is also a place to document what this mean in NeTEx as well. So for the future don´t hasitate to make this kind of improvements. For example: enum TimePoint {ESTIMATE(0), TIMEPOINT(1); int gtfsCode;}.

t2gran

I will approve this, my only comment is relay about the old code, which is using an integer code without any documentation of what 1 and 0 means.

TimMickelsonSkanetrafiken · 2022-02-22T11:58:34Z

I will approve this, my only comment is relay about the old code, which is using an integer code without any documentation of what 1 and 0 means.

@t2gran Please let me know if you want me to document somewhere, that could be done quite quickly I guess?

TimMickelsonSkanetrafiken force-pushed the stoptime-wait-point branch from ab9d46b to 67421f2 Compare February 11, 2022 09:04

MikaelJarfors added the Skanetrafiken On skanetrafikens roadmap label Feb 11, 2022

TimMickelsonSkanetrafiken marked this pull request as ready for review February 11, 2022 09:42

TimMickelsonSkanetrafiken requested a review from a team as a code owner February 11, 2022 09:42

barbeau reviewed Feb 11, 2022

View reviewed changes

t2gran added this to the 2.1 milestone Feb 11, 2022

feat: Map NeTEx WaitPoint to internal StopTime.timepoint

52ae014

- Map from NeTEx to StopTime in StopTimesMapper - Add Unit test in StopTimesMapperTest for all permutations of mapping possibilties

TimMickelsonSkanetrafiken force-pushed the stoptime-wait-point branch from 67421f2 to 52ae014 Compare February 15, 2022 12:10

TimMickelsonSkanetrafiken changed the title ~~New attribute StopTime.waitPoint~~ NeTEx mapping to StopTime.timepoint Feb 15, 2022

t2gran self-requested a review February 17, 2022 14:36

hannesj previously approved these changes Feb 21, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/netex/mapping/StopTimesMapper.java

t2gran reviewed Feb 22, 2022

View reviewed changes

t2gran previously approved these changes Feb 22, 2022

View reviewed changes

Merge branch 'dev-2.x' into stoptime-wait-point

07d06de

TimMickelsonSkanetrafiken dismissed stale reviews from t2gran and hannesj via 07d06de February 22, 2022 11:56

TimMickelsonSkanetrafiken closed this Feb 22, 2022

TimMickelsonSkanetrafiken reopened this Feb 22, 2022

hannesj approved these changes Feb 22, 2022

View reviewed changes

t2gran approved these changes Feb 22, 2022

View reviewed changes

TimMickelsonSkanetrafiken merged commit ff7591f into opentripplanner:dev-2.x Feb 22, 2022

t2gran pushed a commit that referenced this pull request Feb 22, 2022

Add changelog entry for #3898 [ci skip]

177aa91

Uh oh!

Conversation

TimMickelsonSkanetrafiken commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Unit tests

Documentation

Uh oh!

hannesj commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardehrenfried commented Feb 11, 2022

Uh oh!

TimMickelsonSkanetrafiken commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannesj commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barbeau commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barbeau Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

TimMickelsonSkanetrafiken Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

hannesj Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

TimMickelsonSkanetrafiken Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

barbeau commented Feb 15, 2022

Uh oh!

barbeau commented Feb 15, 2022

Uh oh!

TimMickelsonSkanetrafiken commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

t2gran Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

TimMickelsonSkanetrafiken commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TimMickelsonSkanetrafiken commented Feb 11, 2022 •

edited

Loading

hannesj commented Feb 11, 2022 •

edited

Loading

TimMickelsonSkanetrafiken commented Feb 11, 2022 •

edited

Loading

hannesj commented Feb 11, 2022 •

edited

Loading

barbeau commented Feb 11, 2022 •

edited

Loading

TimMickelsonSkanetrafiken commented Feb 16, 2022 •

edited

Loading