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

Sort GTFS files alphabetically inside GtfsModule #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leonardehrenfried
Copy link

This gives us certainty about the order in which the GTFS input files are processed and alleviates the problem of the timezone of a graph file randomly changing.

@leonardehrenfried
Copy link
Author

On top of the unit test I can indeed confirm that by changing the names of GTFS files (and thereby their ordering) I can change the times returned by OTP. This is of course bad, but at least it would be consistent and not random as before.

@leonardehrenfried
Copy link
Author

Since OTP doesn't support multiple time zones, would it even be possible to have two different ones in a single graph?

If you get two GTFS files with a different time zones and one says it's in NY time zone and the other in Denver, would you not have to convert the times in one file to the other time zone?

@robludwig
Copy link

@leonardehrenfried what we've found is that it converts the timezones that are not in the graph timezone to be in the graph timezone. For example it would convert all the Denver times to New York times here.

@markstos
Copy link

Is there a sense about whether the upstream OTP project would be interested in this patch? Making the ordering reliable seems to be all positive.

@robludwig
Copy link

robludwig commented Mar 25, 2020

@markstos we had discussed that already and I gave Leonard the go-ahead to submit the patch if he'd like.

@leonardehrenfried I confirmed with Ben that you can submit this as a patch upstream if you'd like. I can't unilaterally open-source our code so that was an important step I had simply skipped

@markstos markstos changed the base branch from master to ra-fixes March 30, 2020 15:49
@markstos markstos changed the base branch from ra-fixes to master March 30, 2020 15:52
@markstos
Copy link

@leonardehrenfried This PR has been merged into our "ra-fixes" branch, which is itself based on the version of OTP we are actually running in production. I've made ra-fixes the "default" branch in Github so PRs should automatically target that. You should also fork from it when creating future PRs. This branch will work like our own "master" branch, collecting all of our changes as PRs.

It looks like the SHA we are running is about 130 commits behind whatever you forked him. We can look into updating the upstream SHA we are tracking but today when we looked at the 1.5 Changelog we didn't see compelling changes there.

The benefit would be to increase the odds of getting your changes merged upstream. In this case, your patch applied cleanly to the older version we are running so there is no conflict there.

@leonardehrenfried
Copy link
Author

Is there a sense about whether the upstream OTP project would be interested in this patch? Making the ordering reliable seems to be all positive.

I will give it a go.

So far my experience with upstreaming has been mixed. Out of 2 PRs one was accepted and another one merged into OTP 2 but not 1.

@leonardehrenfried
Copy link
Author

opentripplanner#3022

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