Merge pull request #143 from UrbanAnalyst/csa

properly fix #140 in the C++ csa algorithm

Author: mpadge

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: gtfsrouter
 Title: Routing with 'GTFS' (General Transit Feed Specification) Data
-Version: 0.1.4.008
+Version: 0.1.4.009
 Authors@R: c(
     person("Mark", "Padgham", , "mark.padgham@email.com", role = c("aut", "cre")),
     person("Marcin", "Stepniak", , "marcinstepniak@ucm.es", role = "aut",

R/router.R

@@ -247,32 +247,6 @@ gtfs_csa <- function (gtfs, start_stns, end_stns, start_time,
     if (nrow (route) == 0) {
         return (NULL)
     }
-    # Algorithm can generate transfers which transfer back again, because
-    # transfers are incremented at arrival stations, so even on same trip_id,
-    # can still have incremented value as algorithm progresses. These need to
-    # be detected here, and 'max_transfers' reduced to remove intermediate
-    # transfers. This is a very unsatisfactory solution to
-    # https://github.com/UrbanAnalyst/gtfsrouter/issues/140;
-    # full solution will come when #71 is one day implemented.
-    trip_nums <- unique (route$trip_number)
-    back_transfers <- vapply (trip_nums, function (n) {
-        length (unique (diff (which (route$trip_number == n))))
-    }, integer (1L))
-    max_transfers <- max (back_transfers)
-    while (any (back_transfers > 1L)) {
-        max_transfers <- max_transfers - 1L
-        route <- rcpp_csa (
-            gtfs$timetable, gtfs$transfers,
-            nrow (gtfs$stop_ids), nrow (gtfs$trip_ids),
-            start_stns, end_stns, start_time, max_transfers
-        )
-
-        trip_nums <- unique (route$trip_number)
-# Check for transfers back to same trips:
-        back_transfers <- vapply (trip_nums, function (n) {
-            length (unique (diff (which (route$trip_number == n))))
-        }, integer (1L))
-    }
 
     route$trip_id <- gtfs$trip_ids [, trip_ids] [route$trip_number]
 

codemeta.json

@@ -7,7 +7,7 @@
   "codeRepository": "https://github.com/UrbanAnalyst/gtfsrouter",
   "issueTracker": "https://github.com/UrbanAnalyst/gtfsrouter/issues",
   "license": "https://spdx.org/licenses/GPL-3.0",
-  "version": "0.1.4.008",
+  "version": "0.1.4.009",
   "programmingLanguage": {
     "@type": "ComputerLanguage",
     "name": "R",

src/csa.cpp

@@ -250,19 +250,28 @@ CSA_Return csa::main_csa_loop (
                 {
                     size_t trans_dest = t.first;
                     int ttime = csa_in.arrival_time [i] + t.second;
-                    if (ttime <= csa_out.earliest_connection [trans_dest] &&
-                            csa_out.n_transfers [trans_dest] <= csa_pars.max_transfers)
+                    int new_n_transfers = csa_out.n_transfers [csa_in.arrival_station [i]] + 1;
+
+                    const bool time_is_better = ttime < csa_out.earliest_connection [trans_dest];
+                    const bool time_is_equal = ttime == csa_out.earliest_connection [trans_dest];
+                    const bool fewer_transfers = new_n_transfers < csa_out.n_transfers [trans_dest];
+                    const bool same_trip =
+                        csa_out.current_trip [csa_in.arrival_station [i]] == csa_in.trip_id [i];
+
+                    if ((time_is_better || (time_is_equal && fewer_transfers)) &&
+                            new_n_transfers <= csa_pars.max_transfers &&
+                            !(same_trip && new_n_transfers > csa_out.n_transfers [trans_dest]))
                     {
                         DEBUGMSG_CSA("   main loop: incrementing transfer destination " <<
                             trans_dest << " to " <<
-                            csa_out.n_transfers [trans_dest] + 1 << " transfers.",
+                            new_n_transfers << " transfers.",
                             csa_in.departure_station [i]);
 
                         // modified version of fill_one_csa_out:
                         csa_out.earliest_connection [trans_dest] = ttime;
                         csa_out.prev_stn [trans_dest] = csa_in.arrival_station [i];
                         csa_out.prev_time [trans_dest] = csa_in.arrival_time [i];
-                        csa_out.n_transfers [trans_dest]++;
+                        csa_out.n_transfers [trans_dest] = new_n_transfers;
 
                         csa::check_end_stations (end_stations_set,
                                 trans_dest, ttime, csa_ret);