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

Grouping stops #65

Merged
merged 2 commits into from
Oct 5, 2017
Merged

Grouping stops #65

merged 2 commits into from
Oct 5, 2017

Conversation

jamescr
Copy link
Collaborator

@jamescr jamescr commented Aug 10, 2017

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)

@grote
Copy link
Owner

grote commented Sep 16, 2017

@jamescr This has now conflicts since the incofer stops creator doesn't exist anymore.

@jamescr
Copy link
Collaborator Author

jamescr commented Sep 16, 2017

Updated using the default stops creator.

@grote
Copy link
Owner

grote commented Sep 16, 2017

@nlehuby would you mind reviewing this PR?

sys.stderr.write("http://osm.org/relation/" +
str(relation.id) + "\n")
stop_area = StopArea(relation.id, stop_members,
"Stop area without name")
Copy link
Collaborator

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.

Copy link
Collaborator Author

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)
Copy link
Collaborator

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).

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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 ..

Copy link
Collaborator Author

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

# 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)
Copy link
Collaborator

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.

Copy link
Owner

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Owner

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?

Copy link
Collaborator Author

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 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.

Copy link
Owner

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!

@grote
Copy link
Owner

grote commented Sep 25, 2017

@nlehuby Thank you very much for your review! 😃 👍 You found issues I didn't even notice.

@grote grote mentioned this pull request Sep 30, 2017
2 tasks
@grote grote merged commit 13aa7f0 into grote:master Oct 5, 2017
@jamescr jamescr deleted the issue-55b branch October 7, 2017 20:47
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.

3 participants