Skip to content
This repository was archived by the owner on May 27, 2024. It is now read-only.

Review of squashed/cleaned-up frequency-rt-4 branch#2

Open
barbeau wants to merge 2 commits intomasterfrom
frequency-rt-4-squashed
Open

Review of squashed/cleaned-up frequency-rt-4 branch#2
barbeau wants to merge 2 commits intomasterfrom
frequency-rt-4-squashed

Conversation

@barbeau
Copy link
Member

@barbeau barbeau commented Dec 3, 2014

@Mona77 I've "squashed" your commits into a single commit, which makes it simpler to review. I've also cleaned it up a bit by removing all modified files from this commit that didn't directly relate to your work (e.g., where whitespace was changed in a file, and that was the only change).

Can you please take a look at this, check it out and run it, and make sure everything still works as planned?

If so, I'll give you a few final steps to perform where you can create this same commit under your username and submit it as a pull request to the main OTP project.

barbeau
barbeau reviewed Dec 3, 2014
View reviewed changes
src/client/js/otp/modules/planner/ItinerariesWidget.js Outdated
Copy link
Member Author

@barbeau barbeau Dec 3, 2014

Choose a reason for hiding this comment

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

Why are these lines commented out? If they aren't being used, they should just be deleted.

@Mona77
Copy link

Mona77 commented Dec 3, 2014

Thanks Sean!
I have deleted the commented lines, should I pushed it now?

I ran it everything looks fine to me:

​
Also all tests passed:

Tests run: 357, Failures: 0, Errors: 0, Skipped: 0

[INFO]

[INFO] BUILD SUCCESS

[INFO]

[INFO] Total time: 03:15 min

[INFO] Finished at: 2014-12-03T13:39:31-05:00

[INFO] Final Memory: 18M/78M

[INFO] ------------------------------------

Regards,

Mona.

On Wed, Dec 3, 2014 at 8:15 AM, Sean Barbeau notifications@github.com
wrote:

@Mona77 https://github.com/Mona77 I've "squashed" your commits into a
single commit, which makes it simpler to review. I've also cleaned it up a
bit by removing all modified files from this commit that didn't directly
relate to your work (e.g., where whitespace was changed in a file, and that
was the only change).

Can you please take a look at this, check it out and run it, and make sure
everything still works as planned?

If so, I'll give you a few final steps to perform where you can create
this same commit under your username and submit it as a pull request to the

main OTP project.

You can merge this Pull Request by running

git pull https://github.com/CUTR-at-USF/OpenTripPlanner frequency-rt-4-squashed

Or view, comment on, or merge it at:

#2
Commit Summary

  • Add support for GTFS-rt TripUpdates for frequency-based
    (exact_times=0) trips

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#2.

Regards,
Mona

@barbeau
Copy link
Member Author

barbeau commented Dec 3, 2014

@Mona77 Thanks! Yes, please go ahead and push that commit to Github.

Then, you can run the following locally:

  • git branch -D master
  • git fetch origin
  • git checkout master
  • git checkout -b frequency-rt-5
  • git merge frequency-rt-4
  • git reset origin/master
  • git add --all
  • git commit -m "Fix #1347 - Add support for GTFS-rt TripUpdates for frequency-based (exact_times=0) trips"
  • git push origin frequency-rt-5

This will create a new branch frequency-rt-5 that contains a single commit authored by you for all your changes to the project to support GTFS-rt TripUpdates for frequency-based trips.

After you push this to Github, please write up a short list of changes that you needed to make to OTP to add this feature. Then, please ping me with the list by commenting on this issue, and I'll help edit it a bit. Then, you'll be able to open a pull request against the main OTP project - I'll give you instructions for how to do this.

@barbeau
Copy link
Member Author

barbeau commented Dec 3, 2014

BTW, if you want more description about what the above Git commands are actually doing, see the first section of this article.

@Mona77
Copy link

Mona77 commented Dec 4, 2014

@barbeau,Thanks for instructions!

I pushed the new branch to Github. And here are the main modifications:

Please let me know if any of them is not clear.

On Wed, Dec 3, 2014 at 1:36 PM, Sean Barbeau notifications@github.com
wrote:

BTW, if you want more description about what the above Git commands are
actually doing, see the first section of this article
http://makandracards.com/makandra/527-squash-several-git-commits-into-a-single-commit
.

—
Reply to this email directly or view it on GitHub
#2 (comment)
.

Regards,
Mona

@barbeau
Copy link
Member Author

barbeau commented Dec 4, 2014

My edited versions, for formatting initially - we can discuss more in our Hangout:

This pull request adds support for GTFS-rt TripUpdates for frequency-based (exact_times=0) trips, as discussed in opentripplanner#1347.

Excerpt from above GTFS-rt feed, as example input to OTP:

entity {
  id: "1"
  trip_update {
    trip {
      trip_id: "8"
      start_time: "14:41:04"
      schedule_relationship: UNSCHEDULED
      route_id: "D"
    }
    stop_time_update {
      stop_sequence: 13
      arrival {
        time: 1417722769
      }
      stop_id: "701"
    }
    stop_time_update {
      stop_sequence: 14
      arrival {
        time: 1417722949
      }
      stop_id: "241"
    }

Below are details:

  • GTFS-rt TripUpdate arrival/departure times for frequency-based trips should only be provided as absolute times, not delay (since there is no schedule to reference). Also, because frequency-based trips don't have a schedule, the method for estimating missing arrival/departure time is different. In addition to trip_id (which is used for normal schedule-based real-time updates), vehicle_id is used to differentiate trip instances on the same route. For every tripUpdate with different vehicle_id a new tripTimes is created and is added to the timetable of that route. The logic for consuming the real-time updates of frequency-based trips can be seen here.
  • Real-time updates for frequency based trips should have schedule_relationship: UNSCHEDULED header and is consumed differently (code)
  • The tripTimes has been added to scheduledTimetable so that “frequency-based trips with real time update” have the same interface as scheduled-based trips. (code).
  • All data structures of real time updates of frequency based trips should be deleted/reset on every refresh interval (code). This means that only the trip instances reflected in the most recent GTFS-rt update will be used in OTP.
  • No changes have been made to how OTP handles routing for frequency-based trips when no real-time data is available (see code).
  • The web UI has been changed so that the interface for itineraries stays the same in both scheduled based and frequency-based trips (with or without real time updates). Also, the delay value won’t be displayed for frequency-based trips, since delay is not relevant for trips without a schedule (code).
  • For frequency-based trips, OTP assumes each GTFS-rt TripUpdate entity is a distinct trip instance. It is the responsibility of the GTFS-rt feed to make sure that each trip instance is represented correctly. The same trip_id may appear in more than one TripUpdate entity for one route, if there is more than one vehicle serving a route simultaneously (in which case trip_id + vehicle_id would uniquely identify the trip instance). Or, if the route is a loop (in which case trip_id + vehicle_id + start_time would uniquely identify the trip instance - see below).
  • While it doesn't directly affect OTP trip planning, we suggest that the field start_time is included in frequency-based GTFS-rt feeds, in addition to trip_id and vehicle_id. start_time is primarily included to differentiate trip instances on loop routes, where predictions may simultaneously exist for the end of one trip instance and the beginning of the next trip instance for the same vehicle. In this case, the vehicle_id alone may not be sufficient to uniquely identify a trip instance. See this thread for details. start_time should be immutable for a trip instance, meaning that when a new vehicle is running a route, a start_time is generated for that trip instance, and remains the same for the entire lifetime of that trip instance. As mentioned above, because OTP assumes each GTFS-rt TripUpdate entity is a distinct trip instance, OTP does not examine the value of the start_time field in each TripUpdate entity. The GTFS-rt feed is responsible for ensuring that each entity is a distinct trip instance.

@CLAassistant
Copy link

CLAassistant commented Sep 10, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ barbeau
❌ Mona77
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@barbeau @Mona77 @CLAassistant

Comments