Fixed some functions in StandardTripsCreator #126

ialokim · 2018-02-15T18:43:01Z

While working on the countrywide NicaraguaCreator, I've found some issues with the current TripsCreator implementation. See the inline comments for the specific issues.

ialokim

Please see my inline comments.

ialokim · 2018-02-15T18:44:12Z

osm2gtfs/creators/trips_creator.py

@@ -88,7 +88,8 @@ def _prepare_trips(self, feed, schedule, itinerary):
            if input_fr == itinerary.fr and input_to == itinerary.to:
                trip_services = trip["services"]
                for service in trip_services:
-                    services.append(service)
+                    if service not in services:


To prevent the case when the itineraries in the Schedule are split by some reason, do not double the services.

Why should they be split. Sorry I lack of context here.

They are in some cases of the Nicaraguan timetable.json because of the generation by easy-timetable-generator. But anyway, why shouldn't it be supported?

I still don't understand it. Can you explain concretely why they should be split. Sorry for asking this stupid question, I just didn't get it, yet.

I'm referring to the case when there are two or more dict items inside the line's list for the same line, which present different times but using the same service period. See this example:

"lines": { "E-MAN-EST": [ { "exeptions": [], "from": "Mercado Mayoreo", "services": [ "Mo-Su" ], "stations": [ "Mercado Mayoreo", "COTRAN Sur" ], "times": [ [ "05:45", "08:15" ], [ "08:15", "10:45" ], [ "09:15", "11:45" ], [ "10:45", "13:15" ], [ "11:45", "14:15" ] ], "to": "COTRAN Sur" }, { "exeptions": [], "from": "Mercado Mayoreo", "services": [ "Mo-Su" ], "stations": [ "Mercado Mayoreo", "COTRAN Sur" ], "times": [ [ "13:15", "15:45" ], [ "15:15", "17:45" ], [ "13:45", "16:15" ], [ "15:45", "18:15" ] ], "to": "COTRAN Sur" }, ...

Thanks for the explanation. I think this (edge) case is kind of an inconsistency in the standard schedule format and I personally would rather require a clean input format. But I guess if you and @Skippern are in favour of supporting it, there is a deeper reason I still can't see.

But shouldn't then the two dicts not been merged? As far as I see, now the second one (and all that follow) would be just discarded.

now the second one (and all that follow) would be just discarded

To clear it up a bit, I'll explain what happened before and what should happen IMHO:

Before this PR, the services variable contained the following after iterating over the two (or more) dicts for one line: [Mo-Su, Mo-Su]. So in line 98 and more specifically in line 112, it would append the times from both dicts twice to the trips variable with the same service period Mo-Su.

With this change, the service variable would only contain the following: [Mo-Su] and the trips would be getting doubled.

This code isn't about the whole dict that would be discarded, but only preventing the doubling of a service period.

Now I got it. Thanks!

ialokim · 2018-02-15T18:44:51Z

osm2gtfs/creators/trips_creator.py

@@ -125,14 +126,31 @@ def _verify_data(self, schedule, line, itinerary):
                  str(line.route_id) + ")")
            print(" " + itinerary.osm_url)
            print(" " + line.osm_url)
-            return True
+            return False


I'm not 100% sure, but shouldn't it return False when there's an error?

Yes, it looks like this should be False, indeed.

ialokim · 2018-02-15T18:45:19Z

osm2gtfs/creators/trips_creator.py


        # Check if time information in schedule can be found for
        # the itinerary
        if itinerary.route_id not in schedule['lines']:
-            print(" Warning: Route not found in schedule.")
+            print("Warning: Route not found in schedule.")


Just to keep it as in the other cases.

ialokim · 2018-02-15T18:45:39Z

osm2gtfs/creators/trips_creator.py

            return False

+        # Check if from and to tags are valid and same one as first and last itinerary.stop


This explains well the added check.

Good to have this check. However I suggest to reword the comment. Maybe to

Check if from and to tags are valid and correspond to the actual name of the first and last stop of the itinerary.

Changed accordingly.

ialokim · 2018-02-15T18:47:37Z

osm2gtfs/creators/trips_creator.py

@@ -166,16 +184,16 @@ def _add_itinerary_trips(self, feed, itinerary, line, trip_builder,
            gtfs_trip = route.AddTrip(feed, headsign=itinerary.to,
                                      service_period=trip_builder['service_period'])
            trips_count += 1
+            search_idx = 0


To prevent problems in itineraries where a stop (or a stop_name) is served twice, so that it doesn't pick the same time information twice (this would give "high-speed services" for the transitfeed validator), I've introduced the index from where it should search for the next time.

I tested with Managua and it is the same result. Generally checked the code and looks good.

ialokim · 2018-02-15T18:48:37Z

osm2gtfs/creators/trips_creator.py

+                        except ValueError:
+                            pass
+
+                # Make sure the last stop of itinerary will keep being the last stop in GTFS


To solve same problem described above.

ialokim · 2018-02-15T18:49:06Z

osm2gtfs/creators/trips_creator.py

@@ -217,10 +255,10 @@ def _add_itinerary_trips(self, feed, itinerary, line, trip_builder,
                else:
                    try:
                        gtfs_trip.AddStopTime(gtfs_stop)
-                    except ValueError:
-                        print("Warning: Could not add first a stop to trip.")
+                    except transitfeed.problems.OtherProblem:


It never throws ValueError, but OtherProblem.

ialokim · 2018-02-15T18:49:24Z

osm2gtfs/creators/trips_creator.py

                        print(" " + itinerary_stop.name +
-                              " - " + itinerary_stop.osm_id)
+                              " - " + itinerary_stop.osm_url)


just to give more and consistent information

ialokim · 2018-02-15T18:49:46Z

osm2gtfs/creators/trips_creator.py

        for trip in schedule['lines'][itinerary.route_id]:
            trip_services = trip["services"]
            if (trip["from"] == itinerary.fr and
                    trip["to"] == itinerary.to and
                    service in trip_services):
-                times = trip["times"]
+                for time in trip["times"]:
+                    times.append(time)


In order to not loose times (overriding them).

ialokim · 2018-02-15T18:50:05Z

osm2gtfs/creators/trips_creator.py

@@ -323,4 +362,5 @@ def _load_scheduled_stops(self, schedule, itinerary, service):
                        "to"] == itinerary.to and service in trip_services):
                for stop in trip["stations"]:
                    stops.append(stop)
+                break


In order to not duplicate stops.

pantierra

Tested it with Managua, had some minor questions. Looks good. Thanks for improving the standard trips creator!!

pantierra · 2018-02-25T19:22:11Z

osm2gtfs/creators/trips_creator.py

@@ -88,7 +88,8 @@ def _prepare_trips(self, feed, schedule, itinerary):
            if input_fr == itinerary.fr and input_to == itinerary.to:
                trip_services = trip["services"]
                for service in trip_services:
-                    services.append(service)
+                    if service not in services:


Why should they be split. Sorry I lack of context here.

pantierra · 2018-02-25T19:25:21Z

osm2gtfs/creators/trips_creator.py

@@ -125,14 +126,31 @@ def _verify_data(self, schedule, line, itinerary):
                  str(line.route_id) + ")")
            print(" " + itinerary.osm_url)
            print(" " + line.osm_url)
-            return True
+            return False


Yes, it looks like this should be False, indeed.

pantierra · 2018-02-25T19:26:09Z

osm2gtfs/creators/trips_creator.py


        # Check if time information in schedule can be found for
        # the itinerary
        if itinerary.route_id not in schedule['lines']:
-            print(" Warning: Route not found in schedule.")
+            print("Warning: Route not found in schedule.")


pantierra · 2018-02-25T19:37:13Z

osm2gtfs/creators/trips_creator.py

            return False

+        # Check if from and to tags are valid and same one as first and last itinerary.stop


Good to have this check. However I suggest to reword the comment. Maybe to

Check if from and to tags are valid and correspond to the actual name of the first and last stop of the itinerary.

pantierra · 2018-02-26T01:37:24Z

osm2gtfs/creators/trips_creator.py

@@ -166,16 +184,16 @@ def _add_itinerary_trips(self, feed, itinerary, line, trip_builder,
            gtfs_trip = route.AddTrip(feed, headsign=itinerary.to,
                                      service_period=trip_builder['service_period'])
            trips_count += 1
+            search_idx = 0


I tested with Managua and it is the same result. Generally checked the code and looks good.

ialokim · 2018-02-26T22:51:30Z

Tested it with Managua

As Estelí and Managua are currently the only regions that use the standard creator's implementation (please correct me if I am wrong), I think this is ready to be merged too.

Skippern · 2018-02-27T07:03:52Z

I don’t think multiple dicts like this should have a negative impact on the end result. Aun Johnsen

…

On Feb 27, 2018, at 01:06, ialokim ***@***.***> wrote: @ialokim commented on this pull request. In osm2gtfs/creators/trips_creator.py <#126 (comment)>: > @@ -88,7 +88,8 @@ def _prepare_trips(self, feed, schedule, itinerary): if input_fr == itinerary.fr and input_to == itinerary.to: trip_services = trip["services"] for service in trip_services: - services.append(service) + if service not in services: I'm referring to the case when there are two or more dict items inside the line's list for the same line, which present different times but using the same service period. See this example: "lines": { "E-MAN-EST": [ { "exeptions": [], "from": "Mercado Mayoreo", "services": [ "Mo-Su" ], "stations": [ "Mercado Mayoreo", "COTRAN Sur" ], "times": [ [ "05:45", "08:15" ], [ "08:15", "10:45" ], [ "09:15", "11:45" ], [ "10:45", "13:15" ], [ "11:45", "14:15" ] ], "to": "COTRAN Sur" }, { "exeptions": [], "from": "Mercado Mayoreo", "services": [ "Mo-Su" ], "stations": [ "Mercado Mayoreo", "COTRAN Sur" ], "times": [ [ "13:15", "15:45" ], [ "15:15", "17:45" ], [ "13:45", "16:15" ], [ "15:45", "18:15" ] ], "to": "COTRAN Sur" }, ... — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#126 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAw_XEmIH-SoTxLG0l5dCwwPpTnV9wETks5tY0brgaJpZM4SHUS0>.

pantierra

Looks good to me. I'm very happy with these improvements to the standard trips creator. It is great to see that a generic solution, which is used by several cities will improve it all together. Thanks a lot for this!

grote · 2018-03-01T15:21:58Z

Maybe let's get reviews from others whose creators rely on this?

pantierra · 2018-03-01T15:56:26Z

So far I think only the Nicaraguan providers rely on the standard trips creator.

ialokim commented Feb 15, 2018

View reviewed changes

This was referenced Feb 15, 2018

Add support for required via tag (if given inside OSM data) #129

Merged

Add support for national buses and ferries of Nicaragua #130

Open

pantierra reviewed Feb 26, 2018

View reviewed changes

fixed some functions in StandardTripsCreator

32524f0

pantierra added the enhancement label Feb 28, 2018

pantierra approved these changes Feb 28, 2018

View reviewed changes

pantierra added ready High Priority labels Feb 28, 2018

grote merged commit bc7aaa8 into grote:master Mar 1, 2018

ialokim deleted the trips-creator-fixes branch March 3, 2018 15:54

		return False

		# Check if from and to tags are valid and same one as first and last itinerary.stop

Fixed some functions in StandardTripsCreator #126

Fixed some functions in StandardTripsCreator #126

Conversation

ialokim commented Feb 15, 2018

ialokim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantierra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialokim commented Feb 26, 2018

Skippern commented Feb 27, 2018 via email

pantierra left a comment

Choose a reason for hiding this comment

grote commented Mar 1, 2018

pantierra commented Mar 1, 2018