Grouping stops #65
Grouping stops #65
Conversation
|
@jamescr This has now conflicts since the incofer stops creator doesn't exist anymore. |
|
Updated using the default stops creator. |
|
@nlehuby would you mind reviewing this PR? |
core/osm_connector.py
Outdated
| sys.stderr.write("http://osm.org/relation/" + | ||
| str(relation.id) + "\n") | ||
| stop_area = StopArea(relation.id, stop_members, | ||
| "Stop area without name") |
There was a problem hiding this comment.
Maybe use self.stop_no_name instead to let the user configure the name.
| raise RuntimeError("Unknown stop: " + str(stop)) | ||
| # Replace stop id with Stop objects | ||
| # TODO: Remove here and use references in TripsCreatorFenix | ||
| route.stops[i] = self._get_stop(stop, stops) |
There was a problem hiding this comment.
this is a bad pattern, the object type of route.stops is inconsistent:
If you use the default creator, you have a GTFS Stop objects list, if not, you have an OSM2GTFS Stop objects list.
You may want instead to create another object, such as route.gtfs_stops ?
This breaks the Accra creator (that do not use the default creator, and need to get the stops of the routes in the trip creator).
There was a problem hiding this comment.
You are right about the inconsistency but that issue is not part of this PR. The actual "default stops_creator" implementation (already in the master branch) behaves as the fenix and incofer stops_creators, so this inconsistency is inherited from the previous stop creators.
This inconsistency should be a issue to be solve in another pull request.
There was a problem hiding this comment.
this is a bad pattern, the object type of route.stops is inconsistent:
If you use the default creator, you have a GTFS Stop objects list, if not, you have an OSM2GTFS Stop objects list.
After reviewing again the code ( and remembering how it works 🤔 ) I noticed that the inconsistency is not as you said. If you use the default creator you have OSM2GTFS Stop Objects list but if you don't use the default creator you will have a strings list, where each string corresponds to the node ID the corresponds to a stop in OSM.
There was a problem hiding this comment.
you are right. Ok for an issue.
Maybe it is what the TODO at the previous was supposed to mean ..
There was a problem hiding this comment.
Here is the issue #71
creators/stops_creator.py
Outdated
| # Add stop to GTFS object | ||
| schedule.AddStop( | ||
| stop_obj = schedule.AddStop( | ||
| lat=float(stop.lat), | ||
| lng=float(stop.lon), | ||
| name=stop.name, | ||
| stop_id=str(stop.id) |
There was a problem hiding this comment.
maybe add an prefix or suffix to the id ?
You could find some edge cases where a relation has the same id as a node : if this happens, you may have issues as the stop_id must be unique.
There was a problem hiding this comment.
Good point! Only problem is that it would break (my) existing stop IDs. How about only adding a prefix for stop_areas then?
There was a problem hiding this comment.
Ok. I'll add the prefix "SA" to the stop_area only.
There was a problem hiding this comment.
Indeed its going to break Fenix code as it happened with incofer's. Maybe could be fixed as I did in the incofer trips creator.
There was a problem hiding this comment.
@grote I ran fenix code and it works. I think this is because all stop_area relations in floriapa bbox have only one platform element (all relations have 1 stop and 1 platform as members.). With this data the stop_creator will not add stops as parent stations (based stop areas). But just in case, I committed a suggested modification to the trips_creator_fenix in order to manage eventual parent stations.
There was a problem hiding this comment.
Thanks for checking the fenix code @jamescr!
With this data the stop_creator will not add stops as parent stations (based stop areas).
What do you mean by that? The stop_creator will just ignore those stop area relations?
In the code, it looks like len(elem.stop_members) > 1 will still be true in that case.
But just in case, I committed a suggested modification to the trips_creator_fenix in order to manage eventual parent stations.
This change seems to ignore stop areas completely, right?
Incomplete stop area relations might be common in other data sets as well. Shouldn't the default stop creator handle them well then?
There was a problem hiding this comment.
With this data the stop_creator will not add stops as parent stations (based stop areas).
What do you mean by that? The stop_creator will just ignore those stop area relations?
In the code, it looks like len(elem.stop_members) > 1 will still be true in that case.
The overpass query retrieves stops tagged as "platform" and its corresponding "stop_area" relation (with its members). Happens that floriapa "stop_area"' relations has two members, one tagged as "platform" and another tagged as "stop_position". When the StopArea object is created, member tagged as stop_position is ignored because is not a valid stop candidate. Given this, in this case the total StopArea.stop_members is 1.
In this cases, adding the stop_area as a "gtfs stop station" and a stopas a "gtfs stop" makes no sense, that is why the stop_area is ignored and only the stop is added to the stops.txt. If the stop_area is updated with more members tagged as platform the creator will include the stop_area and each stop (as is going on incofer case).
But just in case, I committed a suggested modification to the trips_creator_fenix in order to manage eventual parent stations.
This change seems to ignore stop areas completely, right?
Yes. Was just preventing from a break that will never occur. (I'll explain at the end of the comment).
Incomplete stop area relations might be common in other data sets as well. Shouldn't the default stop creator handle them well then?
Yes, the stop creator would not break if data is incomplete.
The type of the list of stops used in both trip_creators is Stop never a StopArea. That is clear when you look at the stops_creator._get_stop method, always will return a Stop.
Unfortunately for me I tested incofer trip creator with outdated data, when testing fenix with fresh data and back to incofer also with fresh data, everything was ok with the actual trip creators.
There was a problem hiding this comment.
Thanks for the explanation! :)
And now let's get this finally merged!
|
@nlehuby Thank you very much for your review! 😃 👍 You found issues I didn't even notice. |
This PR add support for grouping stops (stations) in the GTFS, as mention in issue #55.
It works for the incofer stops creators and can be easily extended to other creators (even easier when the Pull request #59 get merged)