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

Amtrak and problems with transit bounding boxes for the continental US #1391

Open
walkerke opened this issue Jun 30, 2018 · 7 comments
Open
Assignees

Comments

@walkerke
Copy link

First of all - this is a phenomenal piece of software and I greatly appreciate all of your work on it.

I've been following with interest the incorporation of the bounding box parameter for retrieving transit feeds from TransitLand in valhalla_build_transit. I've noticed an issue with this option, however, when building transit for areas in the continental United States. Amtrak shows up just about anywhere in the continental US - given its large bounding box - and in turn valhalla_build_transit appears to attempt to fetch and build the entire US Amtrak system even if you just want to build transit for a particular area in the US. This takes quite a long time, and is commonly accompanied by a segfault when I've attempted it.

In addition to bounding boxes, TransitLand allows for the fetching of feeds by feed ID, e.g. https://transit.land/api/v1/feeds?tag_key=feed_id&tag_value=albanytransit-or-us. I've forked Valhalla and incorporated this option which is now allowing me to successfully build transit for specific local transit systems in the US. The changes are walkerke@a84d496 and walkerke@ebc76e0.

I wanted to give you a chance to take a look before opening a pull request 1) due to the rapid pace of Valhalla development and 2) to get your input into how it would best fit within your software, given that I wrote it as a bit of a hack to meet my specific needs.

Thanks!

@irees
Copy link
Collaborator

irees commented Jul 11, 2018

hi @walkerke

This is a very useful option. I have one suggestion - can you use the feed's onestop_id instead of the feed_id tag?

Example:
http://transit.land/api/v1/feeds?onestop_id=f-9q9-caltrain

Or comma separated:
http://transit.land/api/v1/feeds?onestop_id=f-9q9-caltrain,f-9q9-bart

@walkerke
Copy link
Author

@irees thanks for your reply! That is a great suggestion, as the comma separated list of multiple feeds would be necessary for regions with overlapping transit systems.

I'll incorporate this and test it out - perhaps it could be used in place of the bounding box parameter (as a bounding box wouldn't be necessary if you already know the onestop IDs). I'll then open up a pull request.

@Rui-Santos
Copy link
Contributor

@walkerke Yeah, the bbox param can give results like the one you described. The main purpose was really to just get individual countries from transit.land instead of the whole world, so I hadn't even thought about that failure mode when trying to just build transit data for a city or smaller region. I'll see if I can find a way to leave out routes/stops,etc. that fall outside the given bounding box. Just afraid that doing such check for each item in a feed will afect transit build times, but I guess we'll never know without doing it and testing. Maybe just checking if a certain feed has limits falling outside the bbox and mark it as such and only do the check for items on such feed(s) will be better.

And now I remember another way the bbox may fail. Given that bboxes are rectangles, near the borders it may catch feeds from other country and add them to the transit data, which is also not good or intended.

@walkerke
Copy link
Author

@Rui-Santos I understand what you mean about keeping transit build times low. My fork includes a parameter that accepts a comma-separated list of onestop IDs which is working for me but isn't ready for a pull request. The bbox parameter works great in the right circumstances; for example I was testing on Honolulu, Hawaii and I was able to use it quite well without having to look up onestop IDs. Perhaps users could supply a list of onestop IDs or a bbox to fetch transit depending on their use case.

@Rui-Santos
Copy link
Contributor

Rui-Santos commented Jul 26, 2018

Perhaps users could supply a list of onestop IDs or a bbox to fetch transit depending on their use case.

That would be a good idea. But I think the way the args are parsed from the command needs to be reworked, for that to be possible. Right now, if you provide an optional arg you need to include all previous optional args, as args are read in the order they are passed in the command, as you may have noticed. The only way I'm seeing to do that without doing the rework I said above is by reading the arg values from the config file instead of parsing them from the build command.

@kevinkreiser
Copy link
Member

Yeah we didnt make use of boost::program_options there are lots of other places where we do this though. please feel free to look at those examples and have a go at adding args in a more robust fashion this way!

@alichass
Copy link

Hi sorry I was just wondering if this bug still exists, namely valhalla_build_transit trying to pull amtrak for entire united states, even if a bounding box is specified

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

No branches or pull requests

5 participants