jmsigner / amt

37 stars 13 forks source link

added buffer for raster::extract #47

Closed bniebuhr closed 2 years ago

bniebuhr commented 3 years ago

Hi!

In this PR I tried to do two things.

  1. First, I added the possibility of using a buffer for extract_covariates, as requested in this issue. Basically I just added ... to the raster::extract call, so it is possible to use the arguments buffer and fun here. Seems to work, even though I am not sure if it is the best solution. I only implemented this for the base extract_covariates functions, for not the _along and time_var ones which seem more complex (and I haven't used so far).

Here a test:

# buffer
deer %>% extract_covariates(sh_forest) # no buffer
deer %>% extract_covariates(sh_forest, buffer = 500, fun = median)
  1. Second, I added the possibility of using terra::extract is case the raster is in SpatRast format, as requested in this other issue. Seems to work, even though for the example of the deer in the package the time of the procedure does not change much (or may be even faster with raster package).

Here is the usage:

sh_forest_terra <- terra::rast(sh_forest)
deer %>% extract_covariates(sh_forest_terra)
deer %>% steps %>% extract_covariates(sh_forest_terra)
deer %>% steps %>% extract_covariates(sh_forest_terra, where = "start")

Here is a time comparison, but maybe this is not the best way of trying it:

# compare time
library(tictoc)
tic()
deer %>% extract_covariates(sh_forest)
toc()

tic()
deer %>% extract_covariates(sh_forest_terra)
toc()

Two remarks here: a. I only implemented this for the base function, not for _along and time_var yet. b. For the terra extraction, there is no buffer argument, so the approach in 1. above will not work. We would have to think of another approach (e.g. having a buffer argument explicitly, then transforming the points on polygons, then extract).

I added these examples above in the examples list for the time being, then we can remove it (or keep it minimal) when this is tested.

Hope this is useful, or at least the beginning of something useful.

bniebuhr commented 3 years ago

Ok, it makes sense to keep a more broad integration to terra to later on. So, for now, I keep only the fast solution for buffer in the extract_covariates function.

I am just not sure how to come back with this now. Should I just change this amd make a new commit in my clone of the repo, then create a new PR? Or maybe i just need to make a new commit, without necessary new PR.... Sorry, I am not used to this formal workflows...

bniebuhr commented 2 years ago

@jmsigner I have now solved the issue, and I kept only the minor changes regarding the additional arguments to use a buffer and removed all changes related to the use of terra. It should be mergeable now, but tell me if there is anytthing else to change.

bniebuhr commented 2 years ago

Hi @jmsigner , even though it seems some checks have failed, I guess it possible to merge that already... It would be useful to use it! =)

jmsigner commented 2 years ago

I will submit a new version to CRAN within the next few days. Thanks for your contribution