ropensci / stplanr

Sustainable transport planning with R
https://docs.ropensci.org/stplanr
Other
417 stars 66 forks source link

Turn off regionalisation in `overline()` #510

Closed joeytalbot closed 1 year ago

joeytalbot commented 1 year ago

Using overline() on a large dataset, the function failed with the default method. But it worked for the same dataset when I turned off regionalisation:

> car_rnet = overline(
+   car_osrm,
+   attrib = "car_driver",
+   regionalise = 1e+07
+ )
2022-12-12 16:29:53 constructing segments
2022-12-12 16:29:59 building geometry
  |++++++++++++++++++++++++++++++++++++++++++++++++++| 100% elapsed=02s  
2022-12-12 16:30:02 simplifying geometry
2022-12-12 16:30:02 aggregating flows
2022-12-12 16:30:08 rejoining segments into linestrings
> car_rnet = overline(
+   car_osrm,
+   attrib = "car_driver"
+ )
2022-12-12 16:32:47 constructing segments
2022-12-12 16:32:54 building geometry
  |++++++++++++++++++++++++++++++++++++++++++++++++++| 100% elapsed=02s  
2022-12-12 16:32:56 simplifying geometry
large data detected, using regionalisation, nrow = 161011
Error in FUN(X[[i]], ...) : subscript out of bounds

It appears to make sense to have regionalisation turned off by default in overline(). Currently the only way to turn it off seems to be to provide a very high number, e.g. regionalise = 1e+07. There is no regionalise = FALSE option.

Robinlovelace commented 1 year ago

Agreed. I think we should set the threshold to a high number as you suggest. Are you happy to give this a go Joey? Thanks for reporting, will save lots of people lots of time I think.

joeytalbot commented 1 year ago

Yes, I'll have a go!

joeytalbot commented 1 year ago

Ok, this issue already seems to be fixed! I was using v1.0.1 of stplanr. In v1.0.2, the latest version, regionalise is set to 1e+09 by default. I should have updated the package :)

Robinlovelace commented 1 year ago

Aha, well spotted. Always worth checking package versions. I think this may be a duplicate of a previous issue. Will close for now and search for the issue.

Robinlovelace commented 1 year ago

The issue was diagnosed and fixed here: https://github.com/ropensci/stplanr/issues/466

Here's how I found it (not easy to find as no mention of regionalisation in the issue): https://github.com/ropensci/stplanr/issues?q=regionalisation