Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 30, 2023

Conversation

JosiahParry
Copy link
Contributor

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
Copy link
Member

Amazing. Will check it out now!

Copy link
Member

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Will have a quick play on this branch and maybe add another example or 2 but overall, huge 👍 and big thanks.

R/linefuns.R Show resolved Hide resolved
R/linefuns.R Show resolved Hide resolved
R/linefuns.R Outdated Show resolved Hide resolved
R/linefuns.R Show resolved Hide resolved
@Robinlovelace
Copy link
Member

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
Copy link
Member

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
Copy link
Member

That's after my changes...

@JosiahParry
Copy link
Contributor Author

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 Robinlovelace merged commit c1726e8 into ropensci:master Sep 30, 2023
5 checks passed
@Robinlovelace
Copy link
Member

Merged 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed-up rnet_merge()
2 participants