fix stitching errors in gtfs ingestion #3943

kevinkreiser · 2023-02-02T05:17:22Z

After merging #3700 i wrote a huge pr description with the work left to do. i will convert that into an issue and we can burn down the tasks but today i bring you the first task already burned down. There were errors about not being able to find the origin/destination stops of some of the stop pairs (the connections between stations where you actually ride on public transit). this makes other things fail. thankfully the problem wasnt all that complex. i'll describe the fix in the diff, comments should as well

ok so the only sad thing about fixing the stitching error was that the error wasnt stitching at all. this means that in a separate pr we need to offset the graph a bit more so that a trip crosses tiles (forcing stitchign to have to happen). i can add this to the list of things we need to do

kevinkreiser · 2023-02-02T05:20:08Z

src/mjolnir/ingest_transit.cc

@@ -234,7 +234,7 @@ std::priority_queue<tileTransitInfo> select_transit_tiles(const boost::property_
 void setup_stops(Transit& tile,
                 const gtfs::Stop& tile_stop,
                 GraphId& node_id,
-                 std::unordered_map<feedObject, GraphId>& stop_graphIds,
+                 std::unordered_map<feedObject, GraphId>& platform_node_ids,


when we write the stops (nodes) into a transit pbf tile we keep track of the gtfs ids assigned to those node which are platforms, thats what this map does. we need that because later we will ingest the part of the feed that references those platforms to connected them with routes/trips at some sort of schedule.

kevinkreiser · 2023-02-02T05:23:17Z

src/mjolnir/ingest_transit.cc

@@ -257,18 +257,29 @@ void setup_stops(Transit& tile,
  auto onestop_id = isGenerated ? tile_stop.stop_id + "_" + to_string(node_type) : tile_stop.stop_id;


to simplify our model of transit we require that every station (even if it doesnt in real life) has:

at least one in/egress

exactl one station

at least one platform

this makes handling connecting the data somewhat easiser and it models the highest level of complexity we want to support (even though bus stops dont need this in almost all circumstances). anyway... on the line above though you'll notice that what we do when the gtfs feed doesnt come with one of these things (either in/egress or platform, as it always has a station) we fake one so that our simplified model is fulfilled. when we fake one we have to give it a unique id so we take its parent stations id and just add the type onto it (assuming real ones one use the type as a name).

kevinkreiser · 2023-02-02T05:25:46Z

src/mjolnir/ingest_transit.cc

+    // other gtfs entities will refer to it. however when it is generated we must use its parents id
+    // and not the generated one because the references in the feed have no idea that we are doing
+    // the generation of new ideas for non-existant platforms
+    platform_node_ids[{tile_stop.stop_id, feed_path}] = node_id;


the problem with giving these generated platforms a new id is that that id is not known to any other entities in the gtfs feed. any schedule information (trips) that connection stations together will connect them by the stations id since from the gtfs feeds point of view there is no platform, valhalla is making that up. so later, when we want to look up with graphid we gave to a platform that we made up by its id, ie when we are iterating over the gtfs feeds trips we need to remember those graphids by their parent station id not their new made up generated id, because of course the feeds raw data doesnt know about that, its a valhalla convention

kevinkreiser · 2023-02-02T05:27:27Z

src/mjolnir/ingest_transit.cc

  }
  node_id++;
 }

+/**


from here down there are pretty much no real important changes, its all me re-naming the generic "stop_graphids" to the specific "platform_node_ids". i think this variable name makes it much more clear what information is going in there even though for the generated ones the gtfs ids will be the parent station (but it is the one to use to get that platforms graphid)

kevinkreiser · 2023-02-02T05:28:27Z

src/mjolnir/ingest_transit.cc

        bool dest_is_generated = tile_info.tile_station_children.find({dest_stopId, currFeedPath}) !=
                                 tile_info.tile_station_children.end();
        auto dest_onestop_id =
            dest_is_generated ? dest_stopId
                              : dest_stopId + "_" + to_string(NodeType::kMultiUseTransitPlatform);
        stop_pair->set_destination_onestop_id(dest_onestop_id);
-        stop_pair->set_origin_graphid(kInvalidGraphId);


the first time i started debugging this problem i thought i could help myself by initializing these to invalid but what i forgot was that this is protobuf 2 so later on it easily just calls has_origin_graphid rather than checking for our sentinel value. same for destination below

kevinkreiser · 2023-02-02T05:29:02Z

test/gurka/test_gtfs.cc

-    .stop_lat = station_one_ll->second.second, .stop_lon = station_one_ll->second.first,
-    .parent_station = stopOneID, .location_type = gtfs::StopLocationType::EntranceExit,
-    .stop_timezone = "America/Toronto", .wheelchair_boarding = "1",
+    .stop_id = stopOneID + "_rotating_door_eh", .stop_name = gtfs::Text("POINT NEMO"),


decided to use a custom id name rather than what valhalla would generate to increase hardness to input from real gtfs

kevinkreiser · 2023-02-02T05:29:30Z

test/gurka/test_gtfs.cc

-    .stop_lat = station_one_ll->second.second, .stop_lon = station_one_ll->second.first,
-    .parent_station = stopOneID, .location_type = gtfs::StopLocationType::StopOrPlatform,
-    .stop_timezone = "America/Toronto", .wheelchair_boarding = "1",
+    .stop_id = stopOneID + "_ledge_to_the_train_bucko", .stop_name = gtfs::Text("POINT NEMO"),


same here, just allowing it to have a non standard id

kevinkreiser · 2023-02-02T05:30:03Z

test/gurka/test_gtfs.cc

@@ -152,6 +152,15 @@ TEST(GtfsExample, WriteGtfs) {
    .stop_timezone = "America/Toronto", .wheelchair_boarding = "1",
  };
  feed.add_stop(thirdStop);
+  struct gtfs::Stop thirdStopPlatform {
+    .stop_id = stopThreeID + "_walk_the_plank_pal", .stop_name = gtfs::Text("THIRD STOP"),


this makes sure to test the case that we can find the graph id for platforms who have actual ids rather than being generated (like the second stations platform above)

kevinkreiser · 2023-02-02T05:31:11Z

test/gurka/test_gtfs.cc

+      stopOneID + "_ledge_to_the_train_bucko",
+      stopTwoID,
+      stopThreeID + "_walk_the_plank_pal",


have to update the trips information to reference the proper stops, you'll see that we reference the two non generated platforms by name but the middle one is generated and will use the parent station id to look up its platform when connecting during pbf generation

kevinkreiser · 2023-02-02T05:31:19Z

test/gurka/test_gtfs.cc

@@ -257,7 +269,7 @@ TEST(GtfsExample, WriteGtfs) {
  EXPECT_EQ(trips[0].trip_id, tripOneID);

  const auto& stops = feed_reader.get_stops();
-  EXPECT_EQ(stops.size(), 5);
+  EXPECT_EQ(stops.size(), 6);


added a platform to the 3rd station above

kevinkreiser · 2023-02-02T05:31:34Z

test/gurka/test_gtfs.cc

+        EXPECT_TRUE(transit.stop_pairs(i).has_origin_graphid());
+        EXPECT_TRUE(transit.stop_pairs(i).has_destination_graphid());


no need to check for bad values, just check that they are actually set

kevinkreiser · 2023-02-02T05:32:01Z

test/gurka/test_gtfs.cc

+  std::string stop_pair_ids[3] = {
+      stopOneID + "_ledge_to_the_train_bucko",
+      stopTwoID + "_" + to_string(NodeType::kMultiUseTransitPlatform),
+      stopThreeID + "_walk_the_plank_pal",
+  };


have to look for the correct names now that they are different

kevinkreiser · 2023-02-02T05:32:14Z

test/gurka/test_gtfs.cc

-    EXPECT_EQ(*stops.find(stopID), stopID);
+    EXPECT_TRUE(stops.find(stopID) != stops.end());
  }
  for (const auto& stop_pair_id : stop_pair_ids) {
-    EXPECT_EQ(*stops.find(stop_pair_id), stop_pair_id);
+    EXPECT_TRUE(stops.find(stop_pair_id) != stops.end());


this was fairly non-sensical

kevinkreiser · 2023-02-02T05:32:28Z

test/gurka/test_gtfs.cc

@@ -385,7 +398,7 @@ TEST(GtfsExample, MakeTile) {

  GraphReader reader(pt.get_child("mjolnir"));
  auto graphids = reader.GetTileSet();
-  int totalNodes = 0;


bad name made it hard to understand

kevinkreiser · 2023-02-02T05:32:45Z

test/gurka/test_gtfs.cc

-  EXPECT_EQ(totalNodes, 9);
+  EXPECT_EQ(transit_nodes, 9);
  EXPECT_EQ(uses[Use::kRoad], 6);
-  // TODO: transit connect edges should only exist between egress and street nodes
-  // EXPECT_EQ(uses[Use::kTransitConnection], 6);
+  // NOTE: there are 4 for every station (3 of those) because we connect to both ends of the closest
+  // edge to the station and the connections are bidirectional (as per usual)
+  EXPECT_EQ(uses[Use::kTransitConnection], 12);
  EXPECT_EQ(uses[Use::kPlatformConnection], 6);
  EXPECT_EQ(uses[Use::kEgressConnection], 6);
-  // TODO: it looks like convert transit skips putting any actual rail edges into level 3
-  // EXPECT_EQ(uses[Use::kRail], 4);
+  // TODO: this is the only time in the graph that we dont have opposing directed edges (should fix)
+  EXPECT_EQ(uses[Use::kRail], 2);


all of these pass now! that means good things!

kevinkreiser added 2 commits February 1, 2023 23:53

fix stitching errors

6d15890

changelog and harden tests/increase test coverage

0610f3e

kevinkreiser commented Feb 2, 2023

View reviewed changes

kevinkreiser requested a review from nilsnolde February 2, 2023 05:35

nilsnolde approved these changes Feb 2, 2023

View reviewed changes

kevinkreiser merged commit 36bd2ea into master Feb 2, 2023

kevinkreiser deleted the kk_stitch_gtfs branch February 2, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix stitching errors in gtfs ingestion #3943

fix stitching errors in gtfs ingestion #3943

kevinkreiser commented Feb 2, 2023 •

edited

Loading

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

kevinkreiser Feb 2, 2023

		@@ -257,18 +257,29 @@ void setup_stops(Transit& tile,
		auto onestop_id = isGenerated ? tile_stop.stop_id + "_" + to_string(node_type) : tile_stop.stop_id;

		EXPECT_TRUE(transit.stop_pairs(i).has_origin_graphid());
		EXPECT_TRUE(transit.stop_pairs(i).has_destination_graphid());

fix stitching errors in gtfs ingestion #3943

fix stitching errors in gtfs ingestion #3943

Conversation

kevinkreiser commented Feb 2, 2023 • edited Loading

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

kevinkreiser commented Feb 2, 2023 •

edited

Loading