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

GTFS Support: Convert PBFs to Transit Tiles #3700

Merged
merged 80 commits into from
Jan 31, 2023
Merged

Conversation

chris-jpark
Copy link
Collaborator

@chris-jpark chris-jpark commented Aug 8, 2022

the basic gist of this pr:

  • is a ton of clean up from the first couple prs
    • there were tons of spots where the id's were being converted from strings to integers but this is not really how the data is supposed to look
    • convert_transit had lots of places where it was incorrectly modifying the graphid to be level 4 (thought it was doing 2 + 1 but
    • we already had 3 for the level so it was getting 4, which messed everything up)
    • also fixed the date time ingestion for the schedule information
    • lots of little changes to things like names and helper functions through out the library. maybe some are overkill please leave a comment if anyone feels strongly and we can remove
  • this also fixes GTFS Support: Connecting to the Road Network #3630 in that we've implemented a way to find the connections to the graph via a naive loki-like search algorithm (doesnt look in adjacent files and isnt that fast but who cares for now)
  • it also allows you to put a lat lon or a way id to influence the place where a station will connect to the road network, at present there is no way to get this from gtfs but we could side load the data (simple csv file?) in the conversion process of gtfs to pbf tiles

In the end the graph is connected between level 3 and level 2 but there are some things that this PR does not do. in fact transit routing still doesnt work but its getting close. here are our next steps in semi-priority order (which i posted down below but feel would be better off in the PR description):

  1. fix stitching in ingest_transit. for whatever reason the stitcher cant find the missing destination/origin for 1 of the stop pairs in the test. the result is we get a couple of logs that say we are bailing on one of the stop pairs. this is because the pair connects across the tile boundary (which is a great thing to test)
  2. make sure convert transit can actually add the edges between stations (actually between stations platforms) so that you can actually route on a kRail or kBus edge
  3. the routing algorithm doesnt work, says it cant find a transit station near by (shouldnt be the case we know the connection does work currently so perhaps). im hoping fixing the first 2 will make this one go away but i expect there to be more routing issues
  4. fix the routing algorithms likely bugs due to the code being unexorcised for years
  5. IF WE MADE IT HERE WE COULD SAY WE ARE IN BETA WITH CAVEATS
  6. hierarchy builder is not fully robust to the presence of transit data, so currently we cant build hierarchies if we want to have transit data as well. this in itself wouldnt stop us from supporting transit but its definitely required to be fixed before transit can be considered fully functional again.
  7. unit test multi feed imports and fix if broken
  8. IF WE MADE IT HERE WE HAVE SOMETHING WE COULD SAY IS OUT OF BETA
  9. transit connect edges are not always formed on both sides of the street network edge, this happens because we currently dont look in adjacent tiles to find the end nodes. ive put a bunch of hints in the code about what to do to fix this (and also get better matches for edges to connect stations to)
  10. clean up the code in convert transit where we hook up the 3 transit node types and connect them to each other. i wrote a big todo in there
  11. another group of tasks to tackle adding transit to the graph after the graph is fully built as mentioned over in: GTFS support after GSoC ? #3806 (comment)

I can turn the above into issues that we can burn down 1 by 1.

@kevinkreiser
Copy link
Member

kevinkreiser commented Jan 30, 2023

Ok so this PR is much closer than it was several months ago however its still not working and there are several problems. ive enumerated these in the pr description. I would recommend that we are ready to merge this as a work in progress that we can burn down the list above to get it into beta and released

@kevinkreiser
Copy link
Member

@nilsnolde @dnesbitt61 @gknisely im happy to review this with you before merging, i know its large so i'd be happy to walk you through it

@nilsnolde nilsnolde self-requested a review January 31, 2023 14:48
nilsnolde
nilsnolde previously approved these changes Jan 31, 2023
Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

Alright, let's (keep) do(ing) this:)

@nilsnolde
Copy link
Member

@kevinkreiser I think CI failing is just a tiny typo when you were hacking in some proto stuff.

@kevinkreiser
Copy link
Member

@nilsnolde yeah, as soon as we get end to end working, we can go back and delete one of the protos and the fetch transit binary. it currently doesnt really work anyway so there is no point in maintaining it after it stops being a useful "illustration" of how it used to work

@nilsnolde nilsnolde merged commit 07724e0 into master Jan 31, 2023
@nilsnolde nilsnolde deleted the gtfs-convert-transit branch January 31, 2023 19:40
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.

GTFS Support: Connecting to the Road Network
3 participants