lulcc icon indicating copy to clipboard operation
lulcc copied to clipboard

"rule should be provided for each neighbourhood map" in CluesModel

Open xameno opened this issue 5 years ago • 7 comments

Hello,

Many thanks to Simon, and to the rest of contributors, for this novel and necessary addition to the R environment!

I can't work around an issue regarding input for the CluesModel function. In specific, I'm including a NeighbRasterStack object for its neighb parameter, and also a numeric with neighbourhood decision rules for its nb.rules parameter. I receive the error: "Error in .local(obs, ef, models, ...) : rule should be provided for each neighbourhood map".

I'm attaching a script based on the pie example of the package. In the documented example in the package material, the nb object created isn't used in the CluesModel (line 36 in the attached script); related, there is no input for the parameters neighb and nb.rules of the CluesModel. I'm including the nb object and the related decision rules in lines 52 and 55 of the attached script, but it seems that the length of the nb.rules vector doesn't match the number of neighbourhood layers of the nb object, which is obviously equal to 3. I've tried different formats and lengths for nb.rules, unsuccessfully though...

I guess I'm missing something due to my inexperience, but could you elucidate?

Many thanks! githubQuestion.txt

xameno avatar May 07 '20 07:05 xameno

Hi! Thanks for your interest in the package. Can you please paste the output of sessionInfo() ?

VLucet avatar May 07 '20 09:05 VLucet

Hi! Thanks for your interest in the package. Can you please paste the output of sessionInfo() ?

Hello, thanks, here you are:

R version 3.6.3 (2020-02-29) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 20.04 LTS

Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0 LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0

locale: [1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C LC_TIME=el_GR.UTF-8
[4] LC_COLLATE=en_US.UTF-8 LC_MONETARY=el_GR.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=el_GR.UTF-8 LC_NAME=C LC_ADDRESS=C
[10] LC_TELEPHONE=C LC_MEASUREMENT=el_GR.UTF-8 LC_IDENTIFICATION=C

attached base packages: [1] stats graphics grDevices utils datasets methods base

other attached packages: [1] lulcc_1.0.4 raster_3.1-5 sp_1.4-1

loaded via a namespace (and not attached): [1] Rcpp_1.0.4.6 lubridate_1.7.8 lattice_0.20-40 png_0.1-7
[5] class_7.3-15 digest_0.6.25 assertthat_0.2.1 ipred_0.9-9
[9] foreach_1.5.0 R6_2.4.1 plyr_1.8.6 backports_1.1.6
[13] acepack_1.4.1 stats4_3.6.3 ggplot2_3.3.0 pillar_1.4.4
[17] rlang_0.4.6 caret_6.0-86 rstudioapi_0.11 data.table_1.12.8
[21] rpart_4.1-15 Matrix_1.2-18 checkmate_2.0.0 gsubfn_0.7
[25] proto_1.0.0 splines_3.6.3 foreign_0.8-75 gower_0.2.1
[29] stringr_1.4.0 htmlwidgets_1.5.1 munsell_0.5.0 xfun_0.13
[33] compiler_3.6.3 pkgconfig_2.0.3 base64enc_0.1-3 htmltools_0.4.0
[37] tcltk_3.6.3 nnet_7.3-13 tidyselect_1.0.0 htmlTable_1.13.3
[41] gridExtra_2.3 tibble_3.0.1 prodlim_2019.11.13 Hmisc_4.4-0
[45] codetools_0.2-16 crayon_1.3.4 dplyr_0.8.5 withr_2.2.0
[49] MASS_7.3-51.5 recipes_0.1.12 ModelMetrics_1.2.2.2 grid_3.6.3
[53] nlme_3.1-144 gtable_0.3.0 lifecycle_0.2.0 magrittr_1.5
[57] pROC_1.16.2 scales_1.1.0 stringi_1.4.6 reshape2_1.4.4
[61] latticeExtra_0.6-29 timeDate_3043.102 ellipsis_0.3.0 generics_0.0.2
[65] vctrs_0.2.4 Formula_1.2-3 lava_1.6.7 RColorBrewer_1.1-2
[69] iterators_1.0.12 tools_3.6.3 glue_1.4.0 purrr_0.3.4
[73] jpeg_0.1-8.1 survival_3.1-8 colorspace_1.4-1 cluster_2.1.0
[77] knitr_1.28

xameno avatar May 07 '20 09:05 xameno

Thanks, I will find some time to look at this this weekend. @simonmoulds you might be more able to help, this is the part of the package I know the least sadly..

VLucet avatar May 08 '20 23:05 VLucet

Right,

I've managed to trace a bug. I think there are two things that have to be edited.

The first thing is in function CluesModel with the only signature ("ObsLulcRasterStack", "ExpVarRasterList", "PredictiveModelList"):

if (!is.null(neighb) && !is.null(nb.rules)) {
        if (length(nb.rules) != length(neighb)) { 
          stop("rule should be provided for each neighbourhood map")
      }
    }

length(nb.rules) is equal to the number of land use categories. Obviously, length(neighb) is incorrect because it returns number of pixels. Instead, we need nlayers(neighb), i.e. one neighbourhood layer for each category. The edit in this line will fix the error "rule should be provided for each neighbourhood map" returned when running the CluesModel with neighbourhood decision rules. But there is another problem when subsequently trying to run the allocate function with the CluesModel object. It returns error "Error in .local(x, ...) : not a valid subset".

The second then thing is in function NeighbRasterStack with signature ("RasterLayer", "ANY", "NeighbRasterStack"). In specific, when the allocate function runs the allowNb function, there is an update of the NeighbRasterStack object at each time point. Problem is that the function NeighbRasterStack with signature ("RasterLayer", "ANY", "NeighbRasterStack"), which is used for this update, returns wrongly a single-layer NeighbRasterStack object always, instead of the desired 3-layer NeighbRasterStack object (one layer for each category). Hence, the error in subset comes from when subsetting the layers of the NeighbRasterStack object. To make it work, I edited the function NeighbRasterStack with signature ("RasterLayer", "ANY", "NeighbRasterStack") as follows:

function (x, weights, neighb, ...) 
{
  categories <- neighb@categories
  weights <- lapply(neighb@calls, FUN=function(neighbLayer) {neighbLayer$w})
  out <- NeighbRasterStack(x=x, weights=weights, categories=categories, fun=mean, ...)
}

Basically, I just retrieve the categories and weights list from the neighb NeighbRasterStack object, and then give it to the correctly working NeighbRasterStack function with signature ("RasterLayer", "list", "ANY"). In this way, we also avoid another error "rule should be provided for each neighbourhood map" which would occur in function CluesModel with the only signature ("ObsLulcRasterStack", "ExpVarRasterList", "PredictiveModelList") due to its following lines:

if (!is.null(neighb)) {
      if (!is(neighb, "NeighbRasterStack")) 
        stop("'neighb' should be an object of class 'NeighbRasterStack'")
      neighb <- NeighbRasterStack(x = obs[[1]], neighb = neighb) 
    }

Again here, if the CluesModel had the neighb NeighbRasterStack object as input, the update in neighb <- NeighbRasterStack(x = obs[[1]], neighb = neighb) would have returned a single-layer NeighbRasterStack object instead of the desired multi-layer for one layer per land category.

Let me know if I can somehow contribute to this. I've never worked on github, but would like to start learning how.

xameno avatar May 09 '20 18:05 xameno

Hi again @xameno and thank you for looking into this in more depth. Im currently preparing and soon attending an online conference and have not found the time to look into this in depth, my apologies. Your first fix makes complete sense, we should replace by nlayers(). Your second edit does make sense too. Will look into it further when time allows, and it would help to have a PR (pull request) at the ready.

In order to contribute and open a PR, you have to:

  1. Fork the repo see here: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-forks
  2. Commit your changes onto your own repo
  3. Open a pull request to merge your changes into this repo's master branch: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request-from-a-fork

VLucet avatar May 10 '20 23:05 VLucet

Hi both, Thanks for looking at this @xameno, and for identifying the bug. This part of the package - to do with neighbourhoods - has caused the most amount of trouble, I think, so it's good that you're getting to grips with it. If you can make a pull request that would be really helpful. Cheers, Simon

simonmoulds avatar May 11 '20 19:05 simonmoulds

Hello both,

I will definitely make a pull request. And I'm leaving it here as a reminder, that the length(neighb) --> nlayers(neighb) edit has to be done to the OrderedModel function as well.

Many thanks, Danis

xameno avatar May 12 '20 14:05 xameno