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

[WIP] Add default route creator #72

Closed
wants to merge 4 commits into from
Closed

[WIP] Add default route creator #72

wants to merge 4 commits into from

Conversation

AltNico
Copy link
Collaborator

@AltNico AltNico commented Oct 4, 2017

The default route creator is based on Incofer's creator and splits up the different properties in small functions which read from the config and the line object by default, but also can be overridden by custom creators easily. This is already done in Incofer's creator but not in Fenix' and Accra's because they do route creation in their trip creators.

Fixes #26.
Fixes #13.

@AltNico
Copy link
Collaborator Author

AltNico commented Oct 11, 2017

OK, so i have rebased and made tests passing here too, so this is ready to be reviewed. Keep in mind that it is based on #70 and therefore only the last commit is interesting (b480997 at the moment).

@AltNico AltNico requested review from grote and jamescr October 11, 2017 02:44
@AltNico AltNico changed the title [WIP] Add default route creator Add default route creator Oct 11, 2017
# Add route information
route = schedule.AddRoute(
route_id=line_ref,
route_type=self._get_route_type(line),
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure it is a good idea to get this from the config, because there might be providers with different route types like bus, tram and trains. Shouldn't this come from OSM ideally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created different small functions for the different properties to, like you proposed, be able to override them in specific creators later. In these functions, I first only got the information from OSM and overrode the functions in Incofer's creator, but then later thought that it would be cool if one could specify the values in the configuration. But with your other comment, I indeed think that this is overkill and not wanted. I just pushed my old commit which is much more simple.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I preffer getting the attributes from OSM. In the case of GTFS optional attributes as fallback we can set a default value for all providers and a warning message with help on how to fix it in OSM. (as an enhacement, when running the script we can set a flag to ignore warnings ).

For required attributes (I also prefer to get them from OSM and printing the error in case of) I'll agree to have the fallback value in the config file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't be better to handle the attributes in core.osm_connector.py?

"text_color": "ffffff",
"type": "Tram",
"url": "http://www.incofer.go.cr/tren-urbano-alajuela-rio-segundo"
},
Copy link
Owner

Choose a reason for hiding this comment

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

This assumes that all lines have the same color, description, etc.
Is this really something for the config? Maybe these should be seen as default values, if no other source of information is available?

Copy link
Owner

Choose a reason for hiding this comment

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

I think @xamanu was advocating for storing these things in OSM, if I remember correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed He was advocating. (I should add that data to osm)

pantierra and others added 4 commits October 24, 2017 22:07
The default route creator is based on Incofer's creator and
splits up the different properties in small functions which
can be overridden by custom creators easily. This is already
done in Incofer's creator but not in Fenix' because Fenix
does route creation in its trips creator.

Fixes #26.
Fixes #13.
@AltNico AltNico changed the title Add default route creator [WIP] Add default route creator Nov 22, 2017
@pantierra
Copy link
Contributor

pantierra commented Nov 29, 2017

I suggest closing this PR and file a new pull request based on the recent work, which has been already successfully used by us (@AltNico, @ialokim and myself) to generate GTFSes in Nicaragua: https://github.com/mapanica/osm2gtfs/tree/default-creators

@AltNico
Copy link
Collaborator Author

AltNico commented Dec 5, 2017

@xamanu

I suggest closing this PR and file a new pull request based on the recent work, which has been already successfully used by us to generate GTFSes in Nicaragua: https://github.com/mapanica/osm2gtfs/tree/default-creators

I agree with you!

@AltNico AltNico closed this Dec 5, 2017
@AltNico AltNico deleted the 26-default-route-creator branch December 23, 2017 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants