UrbanAnalyst / gtfsrouter

Routing and analysis engine for GTFS (General Transit Feed Specification) data
https://urbananalyst.github.io/gtfsrouter/
80 stars 17 forks source link

gtfs_isochrone returns too many stops #38

Closed AlexandraKapp closed 3 years ago

AlexandraKapp commented 3 years ago

I opened another issue on the previously stated problem, because I think this is not related to the transfers issue.

I think the problem is due to this line:

https://github.com/ATFutures/gtfs-router/blob/44f8e43df3dd85c7488e66e33e789967e021dcf6/src/csa-isochrone.cpp#L86

If the actual_end_time never gets updated in line 110 then the break point in line 91 is never reached.

I tried instead to initialize with

int actual_start_time = start_time, actual_end_time = end_time;

and then it seems to work. Not 100% that I'm missing sth else then...


Originally posted by @AlexandraKapp in https://github.com/ATFutures/gtfs-router/issues/14#issuecomment-664946468

another issue that popped up with the custom transfers.txt:

for some stops and times all stops are within reach of 5 minutes. Though, only with the custom transfers.txt, not with the original one:

E.g. with the VBB Feed:

gtfs <- extract_gtfs ("VBB_gtfs.zip")

# this works as expected:
i <- gtfs_isochrone(gtfs, "Boxhagener Platz", day = "Monday", start_time = 8*3600 + 5*60, end_time = 8*3600 + 10*60)

# isochrone with custom transfers
gtfs_cp <- copy(gtfs)
gtfs_cp$stops <- gtfs_cp$stops %>% select(stop_id, stop_lat, stop_lon, stop_name, parent_station) # workaround for bug: # find_xy_cols(obj) :   Unable to determine longitude and latitude columns; perhaps try re-naming columns. "
transfers <- gtfsrouter::gtfs_transfer_table (gtfs_cp, network_times = FALSE)
gtfs_cp$transfers <- transfers

# this produces the implausible isochrone:
i_cust_trans <- gtfs_isochrone(gtfs_cp, "Boxhagener Platz", day = "Monday", start_time = 8*3600 + 5*60, end_time = 8*3600 + 10*60)

nrow(i$mid_points) # returns 6
nrow(i_cust_trans$mid_points) # returns 122233

# with 5 minutes earlier start time this works just fine:
i_cust_trans2 <- gtfs_isochrone(gtfs_cp, "Boxhagener Platz", day = "Monday", start_time = 8*3600, end_time = 8*3600 + 5*60)
nrow(i_cust_trans2$mid_points) # returns 13
mpadge commented 3 years ago

Whoops, closed the wrong issue with the commit which actually fixed this one. Anyway, thanks so much for uncovering what was actually a pretty huge :bug:, which the above commit should have fixed. Another instance of passing R stuff which uses 1-based indices (x[1] as first element) to C++ which uses 0-based indices (x[0] = first), without appropriately converting the indices.

AlexandraKapp commented 3 years ago

I get the same result as you for this query: (https://github.com/ATFutures/gtfs-router/pull/36#issuecomment-674805982):

start_time <- 12 * 3600 + 1200 
end_time <- start_time + 1200 
ic <- gtfs_isochrone (gtfs, 
                      from = "Schönlein", 
                      start_time = start_time, 
                      end_time = end_time) 

ic$midpoints # returns 679

I get the error not for every query, but only for specific ones (I suppose the ones, that never find a starttime ) https://github.com/ATFutures/gtfs-router/blob/44f8e43df3dd85c7488e66e33e789967e021dcf6/src/csa-isochrone.cpp#L107

So e.g. for this query:

ic <- gtfs_isochrone(gtfs,  from = "Boxhagener Platz", start_time = 8*3600 + 5*60, end_time = 8*3600 + 10*60)
mpadge commented 3 years ago

I see no errors:

library(gtfsrouter
gtfs <- extract_gtfs ("./VBB_gtfs.zip")
#> ▶ Unzipping GTFS archive
#> ✔ Unzipped GTFS archive
#> ▶ Extracting GTFS feed✔ Extracted GTFS feed 
#> ▶ Converting stop times to seconds✔ Converted stop times to seconds 
#> ▶ Converting transfer times to seconds✔ Converted transfer times to seconds
gtfs_cp <- data.table::copy (gtfs) # copy used below
gtfs <- gtfs_timetable (gtfs, day = 3)

start_time <- 8*3600 + 5*60
end_time <- 8*3600 + 10*60
from <- "Boxhagener Platz"
ic <- gtfs_isochrone (gtfs, 
                      from = from,
                      start_time = start_time, 
                      end_time = end_time) 
#> Loading required namespace: geodist
#> Loading required namespace: lwgeom
#> Registered S3 method overwritten by 'spatstat':
#>   method     from
#>   print.boxx cli 
#> Linking to GEOS 3.8.0, GDAL 3.0.4, PROJ 6.3.1
ic$start_point
#> Simple feature collection with 1 feature and 2 fields
#> geometry type:  POINT
#> dimension:      XY
#> bbox:           xmin: 13.46097 ymin: 52.51104 xmax: 13.46097 ymax: 52.51104
#> geographic CRS: WGS 84
#>                  stop_name      stop_id                  geometry
#> 1 Berlin, Boxhagener Platz 070101006383 POINT (13.46097 52.51104)
nrow (ic$mid_points)
#> [1] 6
ic$mid_points
#> Simple feature collection with 6 features and 3 fields
#> geometry type:  POINT
#> dimension:      XY
#> bbox:           xmin: 13.44568 ymin: 52.50641 xmax: 13.46976 ymax: 52.51271
#> geographic CRS: WGS 84
#>                                 stop_name      stop_id earliest_arrival
#> 1                 Berlin, Simon-Dach-Str. 070101006384            29640
#> 2 Berlin, Grünberger Str./Warschauer Str. 070101005197            29760
#> 3                    Berlin, Wedekindstr. 070101005982            29820
#> 4                     Berlin, Wismarplatz 070101006472            29700
#> 5      Berlin, Boxhagener Str./Holteistr. 070101006247            29760
#> 6                Berlin, Neue Bahnhofstr. 070101006248            29820
#>                    geometry
#> 1 POINT (13.45677 52.51175)
#> 2  POINT (13.4524 52.51241)
#> 3 POINT (13.44568 52.51271)
#> 4  POINT (13.46368 52.5106)
#> 5  POINT (13.4663 52.50861)
#> 6 POINT (13.46976 52.50641)

# Then same thing using custom transfer table:
gtfs <- data.table::copy (gtfs_cp)
#transfers <- gtfs_transfer_table (gtfs, network_times = FALSE)
#saveRDS (transfers, "transfers.Rds")
transfers <- readRDS ("transfers.Rds")
gtfs$transfers <- transfers
gtfs <- gtfs_timetable (gtfs, day = 3)

ic <- gtfs_isochrone (gtfs, 
                      from = from,
                      start_time = start_time, 
                      end_time = end_time) 
nrow (ic$mid_points)
#> [1] 9
ic$mid_points
#> Simple feature collection with 9 features and 3 fields
#> geometry type:  POINT
#> dimension:      XY
#> bbox:           xmin: 13.44568 ymin: 52.50641 xmax: 13.46976 ymax: 52.51271
#> geographic CRS: WGS 84
#>                                 stop_name      stop_id earliest_arrival
#> 1                 Berlin, Simon-Dach-Str. 070101006384            29640
#> 2 Berlin, Grünberger Str./Warschauer Str. 070101005197            29760
#> 3 Berlin, Grünberger Str./Warschauer Str. 070301009072            29760
#> 4                 Berlin, Simon-Dach-Str. 070101006384            29640
#> 5 Berlin, Grünberger Str./Warschauer Str. 070101005197            29760
#> 6                    Berlin, Wedekindstr. 070101005982            29820
#> 7                     Berlin, Wismarplatz 070101006472            29700
#> 8      Berlin, Boxhagener Str./Holteistr. 070101006247            29760
#> 9                Berlin, Neue Bahnhofstr. 070101006248            29820
#>                    geometry
#> 1 POINT (13.45677 52.51175)
#> 2  POINT (13.4524 52.51241)
#> 3  POINT (13.4524 52.51241)
#> 4 POINT (13.45677 52.51175)
#> 5  POINT (13.4524 52.51241)
#> 6 POINT (13.44568 52.51271)
#> 7  POINT (13.46368 52.5106)
#> 8  POINT (13.4663 52.50861)
#> 9 POINT (13.46976 52.50641)

Created on 2020-08-17 by the reprex package (v0.3.0)

Back to where we were - could you please either paste exact reproducible code, or privately send me the feed and code if its too large? Thanks! (And that reprex uses the feed you previously sent me.)

AlexandraKapp commented 3 years ago

I double checked and now it works for me as well 🤷 might not have pulled the current version properly before. Sorry for that!