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

Accra creator #66

Merged
merged 11 commits into from
Oct 7, 2017
Merged

Accra creator #66

merged 11 commits into from
Oct 7, 2017

Conversation

nlehuby
Copy link
Collaborator

@nlehuby nlehuby commented Aug 16, 2017

  • GTFS routes are created with OSM route_master relations
  • GTFS Trips are created by frequency, using a frequency tag on route_master relations
  • OSM route relations are used to enrich GTFS trips (direction_id, headsign)
  • Stop areas are created (by name and distance)

TODO :

  • use real data for GTFS frequency opening hours
  • use real data for trip travel time

@grote
Copy link
Owner

grote commented Aug 16, 2017

Wow that was fast! I am currently traveling, but looks like there's still two TODOs open anyway.

Before I can look into this in detail, maybe check out the default stop creator implementation in another PR. Maybe it can be re-used/merged with yours. There's also one for stop areas (grouping stops).
For the trip creator, we maybe can move the interpolation method into the default TripsCreator class to reduce code duplication wherever possible.

@nlehuby
Copy link
Collaborator Author

nlehuby commented Aug 17, 2017

Thanks for your feedback.
You can have a look at the generated GTFS, the TODOs are mocks to be removed soon.

About the TripCreator, I've modified the default creator, and refactor the 3 creators accordingly.

About the StopCreator, we are not sure about the correct way to handle it accordingly to your vision :

  • you can create GTFS stops without any grouping (current default creator, seems a fine default behaviour)
  • you can group GTFS stops using OSM StopArea (PR Grouping stops #55 )
  • you can group GTFS stops by name and proximity (current PR, Accra creator)
  • you can mix it all (using OSM Stop Area and grouping them by name and proximity)
    Feel free to have a chat with @prhod in Paris in September about this.

Enjoy your vacations ;)

@prhod
Copy link
Collaborator

prhod commented Sep 6, 2017

We talked about this PR with @grote, and ended with :

  • A default generation using OSM StopArea only
  • A second method that can use both (OSM StopArea and if not available, using distance and/or name)

The name use can be irrelevant in Brazil, because each StopPoint may have unique name (using street names with house numbers).

@nlehuby
Copy link
Collaborator Author

nlehuby commented Sep 25, 2017

As there is no StopArea in Accra I will stick to the name + distance method in this PR.
Maybe you should make an issue so that we have all theses details somewhere when we will refactor the default stop creator ?

@grote
Copy link
Owner

grote commented Sep 25, 2017

As there is no StopArea in Accra I will stick to the name + distance method in this PR.

Sure that makes sense!

Maybe you should make an issue so that we have all theses details somewhere when we will refactor the default stop creator ?

What do you mean exactly?

The idea discussed with @prhod in Paris was to let the default creator use stop area relations, since this is the OSM approach. But then provide another StopCreator that works based on distance. This one doesn't need to live inside the AccraCreator as it is useful for other regions such as Brazil as well. Ideally, it inherits from the other creator or a common super class to reduce code duplication.

@prhod
Copy link
Collaborator

prhod commented Sep 27, 2017

What @nlehuby is saying is that this PR is about the Accra GTFS generator. This PR shouldn't be blocked by the global enhancement of osm2gtfs with the StopArea default creator we mentioned.
Would you agree on this steps :

  • creating an issue about the StopArea default creator (I can do it if you're ok with it)
  • merging this PR to make this accra generator "official" (with possible enhancements later)
  • working on the StopArea default creator afterward, with possible refacto on existing gtfs creators

@grote
Copy link
Owner

grote commented Sep 30, 2017

I didn't mean to say that this PR is blocked by the stop creator discussion. Do you consider it ready to be reviewed and merged?

working on the StopArea default creator afterward

Do you mean to say that you want this to be merged before #65 or will it work either way?

@nlehuby
Copy link
Collaborator Author

nlehuby commented Oct 1, 2017

This PR is ready to be reviewed.

As Accra does not use the stop default creator, the PRs are supposed to be independent, and may be merged in any order.

@jamescr
Copy link
Collaborator

jamescr commented Oct 1, 2017

I agree everything under accra folder is independent, but we should encourage to use as much as possible the default creators.

@nlehuby a couple of doubts:

Do you have a link to OSM wiki or documentation about the default usage of frequency and travel_time tags? I think the usage is very straightforward but is better to have a documentation at hand (just in case).

The frequency tag should not be a param of route instead of route_master?

@nlehuby
Copy link
Collaborator Author

nlehuby commented Oct 2, 2017

frequency and travel_time are not standard tags for now, so they do not have a wiki page.
Do you want me to add a link to the Accra project ? Or to add explanation about the tags in a comment ?

The frequency could have been on the route just as the travel_time tag. But as it is an approximate value of the frequency of the lines, it would have been duplicated on both routes by the contributors.

@jamescr
Copy link
Collaborator

jamescr commented Oct 4, 2017

frequency and travel_time are not standard tags for now, so they do not have a wiki page.
Do you want me to add a link to the Accra project ? Or to add explanation about the tags in a comment ?

Maybe the wiki is a great place for that info. (see issue #12)

The frequency could have been on the route just as the travel_time tag. But as it is an approximate value of the frequency of the lines, it would have been duplicated on both routes by the contributors.

I think frequency is better in the routes (variants) than in the route master. Because the value is not associated to the master, i mean two routes could be related in a route_master but each route with different frequency.

@prhod
Copy link
Collaborator

prhod commented Oct 4, 2017

For the frequency matter, in some cases you are right : the frequency is different from a variation to an other. On the other hand, there are some lines (I think for example of long distance coach) that don't need variations, and the frequency of the forward and backward routes should be the same. Why not setting this property to the route_master ?
Speaking of which, a frequency could also vary on the time of the day ... :-p
Personally, I think both locations are correct, depending on the accuracy expected.

@jamescr
Copy link
Collaborator

jamescr commented Oct 4, 2017

Well, frequency on both route_master and route is OK for me.

@grote
Copy link
Owner

grote commented Oct 4, 2017

Just to let you know that for osm2gtfs and this MR, I personally don't care how you structure the data within OSM.

@grote
Copy link
Owner

grote commented Oct 4, 2017

As Accra does not use the stop default creator, the PRs are supposed to be independent, and may be merged in any order.

StopsCreatorAccra is inheriting from the StopsCreator though.

Well, frequency on both route_master and route is OK for me.

What does it mean then if both have a different frequency?

@jamescr
Copy link
Collaborator

jamescr commented Oct 4, 2017

Well, frequency on both route_master and route is OK for me.
What does it mean then if both have a different frequency?

For me, route variant frequency has priority.

@grote
Copy link
Owner

grote commented Oct 5, 2017

For me, route variant frequency has priority.

Sounds reasonable, but should be explicitly defined somewhere.

@nlehuby this now has merge conflicts with master.

@nlehuby
Copy link
Collaborator Author

nlehuby commented Oct 7, 2017

rebase ok

Copy link
Owner

@grote grote left a comment

Choose a reason for hiding this comment

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

Thanks a lot @nlehuby, great work!

@grote grote merged commit 5bd24ab into grote:master Oct 7, 2017
@nlehuby nlehuby deleted the clean_accra branch October 7, 2017 13:14
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.

4 participants