mlr-org / mlr3spatial

Spatial objects within the mlr3 ecosystem
https://mlr3spatial.mlr-org.com
42 stars 6 forks source link

setCats is deprecated #70

Closed rhijmans closed 2 years ago

rhijmans commented 2 years ago

In the example in ?predict_spatial you use

terra::setCats(stack, layer = "y", value = value)

terra::setCats was deprecated but I cannot remove it because of this reverse dependency. It would help me if you could replace it with

stack <- categories(stack, layer = "y", value = value)

There is also the more similar (identical, in fact)

set.cats(stack, layer = "y", value = value)

But I would suggest using categories because it uses the standard R copy-on-modify semantics. The set. methods such as set.cats do not make a copy, meaning that if there is a copy of stack in the workspace, it will also change. The set. methods can be efficient inside a function when working on a new object, or if you know what you are doing, but I do not recommend it in examples like this.

Thanks!

be-marc commented 2 years ago

Sorry for the late reply. I was on holiday. We changed the example of predict_spatial() in version 0.2.0 to

library(terra, exclude = "resample")

# fit rpart on training points
task_train = tsk("leipzig")
learner = lrn("classif.rpart")
learner$train(task_train)

# load raster and convert to task
stack = rast(system.file("extdata", "leipzig_raster.tif", package = "mlr3spatial"))
task_predict = as_task_unsupervised(stack, id = "leipzig")

# predict land cover classes
pred = predict_spatial(task_predict, learner, chunksize = 1L)

Are you using an old version? We changed this on 18 August 2022.

But I would suggest using categories because it uses the standard R copy-on-modify semantics. The set. methods such as set.cats do not make a copy, meaning that if there is a copy of stack in the workspace, it will also change. The set. methods can be efficient inside a function when working on a new object, or if you know what you are doing, but I do not recommend it in examples like this.

Thanks for checking our code. We internally create a new SpatRater because the original object cannot be used when we parallelize with the future package. So it should be okay to change by reference.

rhijmans commented 2 years ago

I saw this during reverse dependency checking, i.e. for the CRAN version. I should have checked if it had been changed in the dev version. I was referring to set.cats in an example. Inside a function, it is fine if you know what you are doing