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

max_dist and max_dest for points_to_od() #48

Merged
merged 7 commits into from
Aug 19, 2024
Merged

max_dist and max_dest for points_to_od() #48

merged 7 commits into from
Aug 19, 2024

Conversation

mem48
Copy link
Collaborator

@mem48 mem48 commented Aug 19, 2024

Adds two new arguments to points_to_od()

max_dist lets you set the maximum distance e.g. 10,000m useful if you wanted to make a national OD matrix but only for local trips

max_dest lets you set the maximum number of destinations regardless of distance. E..g only to the 10 nearest destinations for each OD.

Both method are very useful for large OD matrices as faster and use less memory.

Benchmark

p = pct::get_centroids_ew()
p = p[1:1000,]

system.time(r1 <- od::points_to_od(p))
user  system elapsed 
   3.31    0.01    3.32 
nrow(r1)
[1] 1000000


system.time(r2 <- points_to_od(p, max_dist = 10000))
user  system elapsed 
   0.22    0.00    0.22 
nrow(r2)
[1] 33930

This scales exponentially so for large ODs e.g. all LSOAs it makes a massive difference.

I've also added st_centriod to p & pd so non-point geometries can be used

@Robinlovelace
Copy link
Member

This looks great. Plan to finalise checks:

  • Benchmark without dist in terms of timing (we can use expand.grid() by default)
  • Check that the outputs of the most common use cases is essentially the same, think about re-ordering
  • Double-check docs

Will aim to do this this evening. Many thanks, then we can port this functionality to {simodels}.

@Robinlovelace Robinlovelace merged commit 75055ac into main Aug 19, 2024
4 checks passed
@Robinlovelace Robinlovelace deleted the max-dist branch August 19, 2024 23:52
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.

2 participants