use spatial indices for sharedPaths() #960
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @rhijmans,
Please ignore this PR until you're back to work after the holiday break. I'm just posting this now, so I don't forget stuff while it's all fresh in my mind.
I've been playing around with the
terra::sharedPaths()
function and it looks extremely useful - thanks so much for adding it! I was wondering if it would be possible to increase its performance, and I noticed that the C++ code doesn't use spatial indices. So, I had a go at updating the C++ code to use spatial indices for theterra::sharedPaths()
calcations. I think I've got it working correctly and it seems that it can now run much faster too, so I thought I'd submit the changes as a PR.Here's a brief description of the changes.
SpatVector::shared_paths()
methods (defined ingeos_methods.cpp
andspat_vector.h
) to have an additional parameter (bool index
). This parameter indicates whether spatial indices should be used or not during processing. This was done to ensure consistency with other functions in the codebase (e.g.,SpatVector::relate()
, https://github.com/rspatial/terra/blob/master/src/geos_methods.cpp#L1319) which have anindex
parameter to indicate if spatial indices should be used.SpatVector::shared_paths()
methods (defined ingeos_methods.cpp
) to use spatial indices (viaGEOSSTRtree_query_r
) when the newindex
parameter istrue
. The implementation is largely based onSpatVector::relate()
(defined ingeos_methods.cpp
). If theindex
parameter isfalse
, then the spatial indices are not used for processing and it basically uses the same C++ code as the current version of the package.callbck
function to the top of thegeos_method.cpp
file. This is so that theSpatVector::shared_paths()
methods can use it. I was getting compilation errors before I moved this function.sharedPaths()
function (defined ingeom.R
) to use the spatial indices by default.Also, to help verify code correctness, I've written a short script (see below). This script compares results when using the spatial indices for processing to results when not using the spatial indices. These tests are based on examples for
sharedPaths()
. Also, I've added a test to verify that the changes improve performance -- otherwise the PR wouldn't really be worth merging -- using a larger spatial dataset. On my computer, the run time decreases from 6 seconds to 0.5 seconds when using the spatial indices.Please let me know if there's any further changes I can make to help get this PR accepted? I'm also happy to explain any of the changes if anythings not clear?
Test script