-
Notifications
You must be signed in to change notification settings - Fork 31
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use self.stop_no_name instead to let the user configure the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll add the prefix "SA" to the stop_area only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 stop
as 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)