Data structure, standard creators, Nicaragua networks #99
Conversation
|
For your information: Currently the Estelí and Managua creators are included in this PR (two json files only). There are also separate PRs for these (#88 and #92). Before merging this in, if desired we can remove them from this PR. For the sake of testing I would just leave them here for now:
|
|
Thanks for this PR! If the two creators are just two json files, leave them in and close the other PRs for them. |
pantierra
changed the title
pantierra
changed the title
osm2gtfs/core/osm_connector.py
Outdated
| members = {} | ||
| for member in stop_area.members: | ||
| if (isinstance(member, overpy.RelationNode) and | ||
| member.role == "platform"): |
There was a problem hiding this comment.
Shouldn't we also accept stop_position? And shouldn't we also accept platforms as ways?
There was a problem hiding this comment.
Yes we should! But this is how it is implemented at the moment (in master), and I would rather change this in a subsequent step (separate PR).
There was a problem hiding this comment.
Yes, please no unrelated changes. This PR is too big as it is already. Changes in existing functionality should be made in separate PRs.
osm2gtfs/core/osm_connector.py
Outdated
|
|
||
| """ | ||
| valid = False | ||
| if 'public_transport' in stop.tags: | ||
| if stop.tags['public_transport'] == 'platform': |
There was a problem hiding this comment.
Why don't we check for stop_position?
There was a problem hiding this comment.
I would say this is not the scope of this PR. It is how it is currently implemented (in master). Please, feel free to open up an issue for this.
There was a problem hiding this comment.
This is treated in #100.
osm2gtfs/core/osm_connector.py
Outdated
| return True | ||
| elif 'amenity' in stop.tags: | ||
| valid = True | ||
| if 'amenity' in stop.tags: | ||
| if stop.tags['amenity'] == 'bus_station': |
There was a problem hiding this comment.
All of this function seems to be very specific to buses, we should include cases for other public transport services too, such as railway=station, railway=tram_stop, ... (I'm sure there are a lot more)
There was a problem hiding this comment.
Yes, I really see here a good point in what you are saying. But I think this should be the topic of a separate PR, as it is not directly connected to the scope of this here.
osm2gtfs/core/stops.py
Outdated
| osm_type = attr.ib(default="relation") | ||
| location_type = attr.ib(default=1) | ||
| osm_url = attr.ib(default="http://osm.org/" + | ||
| str(osm_type) + "/" + str(osm_id)) |
There was a problem hiding this comment.
As in all the other cases, shouldn't we leave that one in __attrs_post_init__ (here duplicated). Plus: Is there any reason you never use https:// instead of http://?
There was a problem hiding this comment.
Yes, I will change.
The reason for using http instead of https is, because when we started using the shorter link, the redirection was only working for http, but I checked and it seemed to be improved on OpenStreetMap.org, so we can perfectly switch here.
osm2gtfs/core/stops.py
Outdated
|
|
||
| stop_id = attr.ib("") | ||
| location_type = attr.ib(default=0) | ||
| osm_url = attr.ib(default=None) |
There was a problem hiding this comment.
Why don't we use an abstract osm-item class with osm_id, tags, name, osm_url and inherit the classes Station, Stop, Itinerary and Line? Or eventually another parent grouping for the first and latter two? Is this possible in combination with attrs?
There was a problem hiding this comment.
That would be also helpful for the _attrs_post_init__ methods.
There was a problem hiding this comment.
This is a good idea. We should check.
There was a problem hiding this comment.
I've seen that the former classes already had inheritance from BaseRoute, at least.
There was a problem hiding this comment.
ok, thanks for the suggestions. Based on this, I did:
- Made all existing data elements
Line,Itinerary,StationandStopinheriting the basic attributes from a new classElement. - Moved add data elements into
elements.py
osm2gtfs/creators/trips_creator.py
Outdated
| times = trip["times"] | ||
|
|
||
| if times is None: | ||
| print("Warning: Couldn't load times from schedule for route") |
There was a problem hiding this comment.
print the route_id and the service string for debugging
There was a problem hiding this comment.
The route_id is printed already in the main function add_trips_to_feed and on the screen the information is present together. The service string is a long list of time information, not sure if this would be nice to print.
| trip_services = trip["services"] | ||
| if (trip[ | ||
| "from"] == itinerary.fr and trip[ | ||
| "to"] == itinerary.to and service in trip_services): |
osm2gtfs/core/helper.py
Outdated
| hex_color = '000000' | ||
| elif color == u'blue': | ||
| hex_color = '0000FF' | ||
| elif color == u'gray': |
There was a problem hiding this comment.
We could support grey here too, couln't we?
There was a problem hiding this comment.
Changed to webcolors. (i hope) they do everything with a magical name_to_hex function.
There was a problem hiding this comment.
Actually I had some thoughts about the whole part of colors. And it turns out to be specific. Now, only Estelí is using it and especially the complementary color function I am not very convinced that it is mature to get into the "Core". So, I moved it to a newly created routes_creator_esteli.py.
There was a problem hiding this comment.
Yes, the colour tag from OpenSrreetMap is supported and for example Managua uses it, too (using hex values as they are also specified on OSM for the colour tag). I just suggested to move two things to the Estelí creator:
- conversion from name to hex: about the introduced function to map names and hex I was not convinced, so I thought webcolors was a good idea. But it is another dependency and I'm not sure really most creators may use it. Currently it is only Estelí. @grote suggested to do this in a separate issue.
- generation of a complementary color: first it wasn't working... The result was always #000000. Then I checked it briefly, and realized that I'm doubting the "complementary" color is what we need. I think we need more the color with the highest contrast, which seems to be something different. I think it is not that easy than the function proposes and I would prefer to handle it also in a new issue.
There was a problem hiding this comment.
I opened up an issue to discuss this: #104
osm2gtfs/core/osm_connector.py
Outdated
| else: | ||
| rv = result.get_relations(member.ref) | ||
| if bool(rv): | ||
| rv = rv.pop() | ||
| sys.stderr.write("Route variant was assigned again:\n") | ||
| sys.stderr.write("Itinerary was assigned again:\n") | ||
| sys.stderr.write( | ||
| "http://osm.org/relation/" + str(rv.id) + "\n") |
There was a problem hiding this comment.
Here and in all the other cases, we should use an static variable to not hardcode the same url over and over again? Also, please use https.
There was a problem hiding this comment.
I changed to https. However on using a static variable or a global constant and how to handle those concepts in Python is not completely clear to me. A quick research got me to the point, that we probably will need some kind of const class, which will bring a lot of good oportunities. But it seems to be bigger and we need to think about it. What about doing this in a separate issue? So far they have always been hardcoded, so maybe let's change it properly after this going in. What do you think, @ialokim?
There was a problem hiding this comment.
I'm okay with this. 👍 Could you open the new issue?
osm2gtfs/core/stops.py
Outdated
| "http://osm.org/" + self.osm_type + "/" + str( | ||
| self.osm_id) + "\n") | ||
| sys.stderr.write("http://osm.org/" + identifier + "\n") | ||
| sys.stderr.write("http://osm.org/" + self._parent_station + "\n") |
There was a problem hiding this comment.
Use osm_url here and above.
There was a problem hiding this comment.
This is not possible, because we are not getting full objects, but rather the parent station is the GTFS stop_id, which people likely want to override in their stops_creator. Given this I think the error messages leading to OSM objects, are not a good idea here. Taking them out.
osm2gtfs/core/helper.py
Outdated
| return center_lat, center_lon | ||
|
|
||
| @staticmethod | ||
| def get_hex_code_for_color(color): |
There was a problem hiding this comment.
That's already a pretty extensive list but I think we should at least support the colors mentioned in the OSM wiki.
There was a problem hiding this comment.
I would replace it with webcolors
There was a problem hiding this comment.
Like that idea! Should it be another PR?
There was a problem hiding this comment.
Yes, the whole color transformation (from name to hex; from one color to a high contrast color) is probably better to handle separately: #104
osm2gtfs/core/osm_connector.py
Outdated
| # Collect the Stop objects that are members | ||
| # of this Station | ||
| members["node/" + str(member.ref)] = self.stops[ | ||
| 'regular']["node/" + str(member.ref)] |
There was a problem hiding this comment.
You could create a variable for "node/" + str(member.ref) to not build it three times.
There was a problem hiding this comment.
Or even three static variables for the three osm types?
osm2gtfs/core/routes.py
Outdated
| variants of the same service line. | ||
|
|
||
| In OpenStreetMap this is usually represented as "route_master" relation. | ||
| In GTFS this is usually represented as "route" |
There was a problem hiding this comment.
Sentence should end with a . like the other ones.
| from math import cos, sin, atan2, sqrt, radians, degrees | ||
|
|
||
|
|
||
| class Helper(object): |
There was a problem hiding this comment.
Great idea with the Helper class!
osm2gtfs/core/routes.py
Outdated
| # Related route variants | ||
| _itineraries = attr.ib(default=attr.Factory(list)) | ||
|
|
||
| def __attrs_post_init__(self): |
There was a problem hiding this comment.
Great idea with this __attrs_post_init__!
| def add_stops_to_routes(self, data): | ||
| routes = data.routes | ||
| stops = data.stops | ||
| if type(stop_name).__name__ == "unicode": |
There was a problem hiding this comment.
Unicode handling in Python (2) is so ugly...
osm2gtfs/creators/stops_creator.py
Outdated
|
|
||
| if "gtfs_stop_id" in stop.tags: | ||
| # Use a GTFS stop_id coming from OpenStreetMap data | ||
| return stop.tags['gtfs_stop_id'] |
There was a problem hiding this comment.
Is the usage of gtfs_stop_id in OpenStreetMap documented?
There was a problem hiding this comment.
No, it is not. If people here are very strong on taking it out. I'm fine with it. I just think it is a nice feature, especially when GTFS already exist in an area and the stops should map to those ids.
There was a problem hiding this comment.
I don't see the use cases for defining their own stop IDs, but if someone needs it, I'm fine with it. At least I would document it somewhere in the osm2gtfs wiki.
There was a problem hiding this comment.
In Grande Vitória, most bus stops have a ref defined by municipal or operator, usually posted on the bus stop sign, all known such references are mapped with the ref tag. This could be used instead of automatically get the OSM object ID. Making an independent gifts_stop_id tag will only duplicate data and result in data not being added.
There was a problem hiding this comment.
Most people who work with GTFS appear to be kind of holy about their ids (think about porting an existing GTFS to OSM and then being able to see what's happening), and I can understand, because it is the reference to the thing. Yes, @Skippern, ref seems to be the correct way to go, maybe even also supportin ref:gtfs for special needs. gtfs_stop_id was a bad idea. Thanks for noticing, will change!
There was a problem hiding this comment.
Changed, accordingly.
| 'stops': scheduled_stops, 'schedule': trips_schedule}) | ||
| return trips | ||
|
|
||
| def _verify_data(self, schedule, line, itinerary): |
There was a problem hiding this comment.
Great method that you've introduced here!
osm2gtfs/creators/trips_creator.py
Outdated
| try: | ||
| gtfs_trip.AddStopTime(gtfs_stop) | ||
| except ValueError: | ||
| print("Warning: Could not add a stop to trip.") |
There was a problem hiding this comment.
If I remember correctly transitfeed only throws this ValueError when you try to add the first stop of a trip without time. We could adjust the message to include that information.
| gtfs_service = ServicePeriod(service) | ||
| gtfs_service.SetDateHasService(service) | ||
| else: | ||
| raise KeyError("Unknown service keyword: " + service) |
There was a problem hiding this comment.
Doesn't this should also handle range of ISO dates like specified in the wiki?
There was a problem hiding this comment.
@AltNico, Do you think this is a better to be handled in a new issue? Or should it be treated here?
There was a problem hiding this comment.
I think a new issue would be better.
README.md
Outdated
| * [Florianópolis, Brazil](https://github.com/grote/osm2gtfs/blob/master/osm2gtfs/creators/fenix/fenix.json) | ||
| * [Suburban trains in Costa Rica](https://github.com/grote/osm2gtfs/blob/master/osm2gtfs/creators/incofer/incofer.json) | ||
| * [Accra, Ghana](https://github.com/grote/osm2gtfs/blob/master/osm2gtfs/creators/accra/accra.json) | ||
| * [Managua, Ciudad Sandino](https://github.com/grote/osm2gtfs/blob/master/osm2gtfs/creators/managua/managua.json) and [Estelí](https://github.com/grote/osm2gtfs/blob/master/osm2gtfs/creators/esteli/esteli.json) in Nicaragua |
There was a problem hiding this comment.
Better to use relative links here. I think the wiki links above are fine absolute, but these here refer to files in the repo, so they should be relative.
osm2gtfs/core/osm_connector.py
Outdated
| members = {} | ||
| for member in stop_area.members: | ||
| if (isinstance(member, overpy.RelationNode) and | ||
| member.role == "platform"): |
There was a problem hiding this comment.
Yes, please no unrelated changes. This PR is too big as it is already. Changes in existing functionality should be made in separate PRs.
|
I've fixed two bugs found while generating the (almost complete now) GTFS for Esteli:
|
|
Thanks, @ialokim! Glad to hear that the Estelí GTFS is reaching completeness. Looking forward to have public transport routing in Nicaragua for the first time and already in three cities! Yeay |
|
Thanks for all the feedback so far! I think I processed all of it and made sure all networks build properly. In case you have already tested this PR, please make sure you execute (once) with the You should be easily able to build all networks, without preparing anything, just run:
Thanks everybody. |
osm2gtfs/core/elements.py
Outdated
| self.route_color = self.tags['colour'] | ||
|
|
||
| # pylint: disable=unsupported-membership-test,unsubscriptable-object | ||
| if 'ref:colour_tx' in self.tags: |
There was a problem hiding this comment.
ref:colour_tx is not a standard tag (I've never heard of this one before today ;) ), you will not find it in every route_master relations: it should be handled in the creators (just like the travel_time and frequency tags used in Accra.
There was a problem hiding this comment.
I also haven't heard about ref:colour_tx before 😄 Looking in the wiki I found it the other day and thought it would be probably the most appropriate. Yes, I agree, it can also be handled in the routes_creator.py, this is also totally fine.
osm2gtfs/core/elements.py
Outdated
| elif 'route' in self.tags: | ||
| self.route_type = self.tags['route'].capitalize() | ||
| else: | ||
| self.route_type = "Bus" |
There was a problem hiding this comment.
We should add a log for the route_master relations that do not have route_master tag: this is an issue that should be fixed in OSM.
There was a problem hiding this comment.
Yes, makes sense. Added.
osm2gtfs/core/elements.py
Outdated
| a Line object. | ||
|
|
||
| In OpenStreetMap this is usually represented as "route" relation. | ||
| In GTFS this is not exlicitly presented but used as based to create "trips" |
There was a problem hiding this comment.
exlicitly ? used as based ? 
There was a problem hiding this comment.
Thanks, for noticing!
osm2gtfs/core/elements.py
Outdated
| self.to = self.tags['to'] | ||
|
|
||
| # pylint: disable=unsupported-membership-test,unsubscriptable-object | ||
| if 'duration' in self.tags: |
There was a problem hiding this comment.
duration is not a standard tag you will find in all route_master relations: it should be handled in the creators (just like the travel_time and frequency tags used in Accra.
There was a problem hiding this comment.
duration is IMHO the standard tag. Ah, and if it is not defined on OSM, it will just fall back to None on the data object, and can and might be handled later in the creators as you suggest.
There was a problem hiding this comment.
The more I think through it the more I follow your approach. Mainly because there has been so many different and valid ideas on how to handle it: travel_time or duration tag in OSM. In the other issue they also requested to define this in the schedule source. So, yes, will move it to the schedule creators. Thanks for rising it.
| This function adds the routes from the data to the GTFS feed. | ||
| """ | ||
| # Get route information | ||
| routes = data.get_routes() |
There was a problem hiding this comment.
It is unclear if we are talking about OSM routes or GTFS routes. Shouldn't we call this var lines instead of routes ?
There was a problem hiding this comment.
Actually they contain both Lines (OSM Route Masters) and Itineraries (OSM Routes), when they appear together in code they are just called routes (like in the concept of GTFS).
There was a problem hiding this comment.
As I understand correctly, this variable only contains Lines objects containing Itineraries objects. Wouldn't it be easier to understand by making this explicit with a lines variable ?
osm2gtfs/creators/routes_creator.py
Outdated
|
|
||
| def _define_route_color(self, route): | ||
| """ | ||
| Returns the route_route_color for the use in the GTFS feed. |
osm2gtfs/creators/stops_creator.py
Outdated
| # Send stop_id creation through overridable function | ||
| gtfs_stop_id = self._define_stop_id(stop) | ||
|
|
||
| # Save defined stop_id to the object for furhter use in other creators |
osm2gtfs/creators/trips_creator.py
Outdated
| times = None | ||
| for trip in schedule['lines'][itinerary.route_id]: | ||
| trip_services = trip["services"] | ||
| if (trip[ |
There was a problem hiding this comment.
Odd place to break the line 🤔 Why not ?
if (trip['from'] == itinerary.fr
and trip['to'] == itinerary.to
and service in trip_services):
There was a problem hiding this comment.
Because my linter complains. Is it just mine? or I can do as you say? Which would be much nicer, indeed!
There was a problem hiding this comment.
Just tried with your proposal and unfortunately fails here in the CI, too:
osm2gtfs/creators/trips_creator.py:319:17: E129 visually indented line with same indent as next logical line
Please help me to produce a nicer readable line that sticks to our coding standards. The one in code was the only one that worked for me, without breaking the CI.
There was a problem hiding this comment.
This one seems ok :
if (trip["from"] == itinerary.fr and
trip["to"] == itinerary.to and
service in trip_services):
times = trip["times"]
There was a problem hiding this comment.
hey, yes @nlehuby this works and is much better. Thanks!
|
Thank you, @nlehuby! |
Author: Felix Delattre <github@xama.nu>
|
Checks fail now, but is not caused by this Pull Request. Compare the same latest commit to upstream with exactly the identical recently pushed to another repository. Locally all tests work fine. No idea how to fix it. |
osm2gtfs/core/osm_connector.py
Outdated
| "Error: Station with no members has been discarted:\n") | ||
| sys.stderr.write("https://osm.org/relation/" + | ||
| str(stop_area.id) + "\n") | ||
| return False |
There was a problem hiding this comment.
This method is sometimes returning None and sometimes False. This should be unified.
osm2gtfs/creators/trips_creator.py
Outdated
| sys.stderr.write( | ||
| "Itinerary (" + itinerary.route_url + ") misses a stop: \n") | ||
| sys.stderr.write( | ||
| "Please review:" + itinerary_stop_id + "\n") |
|
I went over the code again, rebased in on upstream's master and included suggestions and comments; also the "real problem" should be fixed by now. I would be happy to merge the current state in. |
|
Thanks for making the fixes so quickly. Looks good to me now. However, we shouldn't merge this as long as the CI is failing. It seems the problem is here: So the size of stop_times changed. Unfortunately, this is not very helpful in inspecting the differences. Maybe we can run diffoscope on it? The change might also come from today's |
|
Ok, CI is failing on master anyway, most likely due to the new transitfeed library. |
This is a new pull request for the overhauled data structure, now with standard creators and Nicaragua config files that rely on these. It comes with a lot of wonderful work by @AltNico and @ialokim.
Following a request of @ialokim, I'm opening a PR already. It is meant to be subject for review by all people involved in osm2gtfs.
This PR addresses the following issues: