ropensci / stplanr

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

Conditionally use rsgeo::line_segmentize() if available #542

Closed JosiahParry closed 9 months ago

JosiahParry commented 9 months ago

Closes #522.

This PR speeds up line_segment() by utilizing rsgeo. This only occurs when the package is installed (there is no prompting introduced in this PR as might be done using rlang::check_installed()), and the input geometry either has a missing CRS or is not a geographic CRS this is because rsgeo utilizes euclidean distance to segmentize. If there is a desire to use geodesic / haversine distance in a line_segmentize function, theres potential to add that upstream. However, its my understanding that most road network anlaysis is done on a fairly localized level which should be done using a projected CRS anyways! :)

Further, this PR converts line_segment() into an S3 generic with an sf and sfc_LINESTRING method enabling people to work with only their geometries and not always have to have a data frame.

rsgeo is added to the DESCRIPTION's Suggests field.

Benchmark:

library(stplanr)
l <- routes_fast_sf[which(n_vertices(routes_fast_sf) != 2), ] 

bench::mark(
  planar = line_segment(sf::st_set_crs(l, NA), 5),
  og = line_segment(l, 5),
  check = FALSE
)
#> Linking to GEOS 3.11.0, GDAL 3.5.3, PROJ 9.1.0; sf_use_s2() is TRUE
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 planar       2.52ms   2.78ms    271.      1.62MB     37.9
#> 2 og         356.14ms 358.39ms      2.79      30MB     14.0
Robinlovelace commented 9 months ago

Amazing. Will check it out now!

Robinlovelace commented 9 months ago

Straight back at ya, minor changes suggested including exposing use_rsgeo, will be useful for benchmarking: https://github.com/JosiahParry/stplanr/pull/1/files

Robinlovelace commented 9 months ago

For some reason I get this note, not sure if it's new, but should add to the global allowed names I'm thinking to make checks happier:

  Undefined global functions or variables:
    segment
Robinlovelace commented 9 months ago

That's after my changes...

JosiahParry commented 9 months ago

For some reason I get this note, not sure if it's new, but should add to the global allowed names I'm thinking to make checks happier:


  Undefined global functions or variables:

    segment

This is likely because there is a variable declared an unused? I won't be able to review for a few more hours.

Robinlovelace commented 9 months ago

Merged :+1: