Code redundancy in R5TravelTimeComputer

[rafapereirabr]

Issue kindly pointed out by @abyrd in this discussion conveyal/r5#934, more specifically here. @mvpsaraiva , could you please have a look at this ?

[rafapereirabr]

@BardyBard , here's a the code for a reprex that might help you investigate further this issue.

devtools::load_all('.')
library(dplyr)

# build transport network
data_path <- system.file("extdata/poa", package = "r5r")
r5r_core <- setup_r5(data_path = data_path)

# load origin/destination points
points <- read.csv(file.path(data_path, "poa_hexgrid.csv"))

# sample a few poits to make reprex faster
set.seed(42)
points20 <- dplyr::sample_n(points, size = 20)

# load fare structure object
fare_structure_path <- system.file(
  "extdata/poa/fares/fares_poa.zip",
  package = "r5r"
)

fare_structure <- read_fare_structure(fare_structure_path)


# set departure time
departure_datetime <- as.POSIXct(
  "13-05-2019 14:00:00",
  format = "%d-%m-%Y %H:%M:%S"
)

# run pareto frontiers at different max walk times

# 10 minutes walking at 5 Km/h = equivalent to ~830 meters
test_00010 <- pareto_frontier(
  r5r_core,
  origins = points20,
  destinations = points20,
  mode = c("WALK", "TRANSIT"),
  departure_datetime = departure_datetime,
  fare_structure = fare_structure,
  fare_cutoffs = c(4.8, 9), 
  max_walk_time = 10,
  walk_speed = 5, 
  progress = TRUE
)

# 24000 minutes walking at 5 Km/h = equivalent to 2000 meters
test_24000 <- pareto_frontier(
  r5r_core,
  origins = points20,
  destinations = points20,
  mode = c("WALK", "TRANSIT"),
  departure_datetime = departure_datetime,
  fare_structure = fare_structure,
  fare_cutoffs = c(4.8, 9), 
  max_walk_time = 24000,
  walk_speed = 5, 
  progress = TRUE
)

# 36000 minutes walking at 5 Km/h = equivalent to 3000 meters
test_36000 <- pareto_frontier(
  r5r_core,
  origins = points20,
  destinations = points20,
  mode = c("WALK", "TRANSIT"),
  departure_datetime = departure_datetime,
  fare_structure = fare_structure,
  fare_cutoffs = c(4.8, 9), 
  max_walk_time = 36000,
  walk_speed = 5, 
  progress = TRUE
)



head(test_00010)
head(test_24000)
head(test_36000)

# check wether output sizes are different as excpected
# the output should be larger when we allow for longer walking distances

# this one is on
nrow(test_00010) < nrow(test_24000)

# this one is not
nrow(test_24000) < nrow(test_36000)

# in fact, these two ouputs are identical, which suggests that the limit of 2Km is still in effect
identical(test_36000, test_24000)

[BardyBard]

@rafapereirabr I've tested this code on v.0.7.1, the first release with commit #fea62c6, using all 1222 data points in the dplyr dataset, and you are correct the results for 2km and 3km of max distance are identical.

So either:

1. This dataset has no points where you would walk for more than 2km after using transit (which I don't understand enough about r5r to test)
2. We are interpreting @mvpsaraiva commit message incorrectly. Marcus, do you by chance recall what this code duplication and bugfix was about?

Ill await Marcus' response on whether he has any extra information about his commit.

[mvpsaraiva]

Hi all.

I'm revisiting all this to remember the reasons of the code duplication. My commit message was:

Fixing bug in TravelTimeComputer where R5 always considered max_walk_dist = 2000 when using a Fare Calculator.

All the duplicated code was added just to change a call to create a router object. My code is:

while the original R5 code is:

Basically, the accessTimes parameter is always set to null originally, which means the walking times to the transit stations previously computed are ignored in that situation. When this happens, R5 recalculates those access times again in McRaptorSuboptimalPathProfileRouter, but this time using the constant TransitLayer.WALK_DISTANCE_LIMIT_METERS, which is set to 2000 m:

That's why the results for 2km and 3km of max distance are identical without this bug fix. This should be simple to fix upstream, and then we can remove this duplicated code.

[rafapereirabr]

Hi @abyrd and @ansoncfit . Please have a look at @mvpsaraiva's comment above. Do you think this could be be a issue in R5 that deserves your attention ?

[BardyBard]

Hi @mvpsaraiva, I understand that the duplication was to remove the default 2km from McRaptorSuboptimalPathProfileRouter.

@rafapereirabr made this test code to check whether the results for 2km and 3km are different when running pareto_frontier.

I've tested the code on R5R v0.7.1, the first version after your code duplication commit and I get no difference in results between 2km and 3km. There is also no difference in results in the current version of R5R as mentioned by @rafapereirabr.

This either means that the test code is incorrect or that the code duplication didn't solve the issue.

[mvpsaraiva]

Hi @BardyBard.

I think the test code is a bit odd... max_walk_time is set to 10 minutes in the first test, but then it goes to 24000 and 36000 in the third. I think @rafapereirabr mixed the units and the correct values are 10, 24 and 36. Try running the code with those values to see if there's any difference.

[BardyBard]

You are completely correct about the units. Sorry about that, should have noticed the the time is way too long. Everything works as expected now.

[rafapereirabr]

we have rerun the analysis and this has been fixed using the latest version of R5 7.3