inlabru-org / inlabru

inlabru
https://inlabru-org.github.io/inlabru/
76 stars 21 forks source link

Bug in gg.SpatialPixelsDataFrame: handling of factors #60

Closed courtiol closed 3 years ago

courtiol commented 5 years ago

Dear developers,

In inlabru 2.1.9 (but not in the current devel), there is a bug in gg.SpatialPixelsDataFrame(). The condition if (!inherits(data[[deparse(dmap$fill)]], "factor")) is never TRUE because a call to deparse() does not return the original name when being evaluated on a quosure (with deparse a tilda is added to the name, since closure are actually formulas).

Ex:

deparse(quo(test)) == "test"
# [1] FALSE

That makes the function gg.SpatialPixelsDataFrame() crash:

library(inlabru)
library(INLA)
data(gorillas, package = "inlabru")
matern <- inla.spde2.pcmatern(gorillas$mesh,
                              prior.sigma = c(0.1, 0.01),
                              prior.range = c(5, 0.01))
cmp <- coordinates ~ mySmooth(map = coordinates, model = matern) + Intercept
fit <- lgcp(cmp, gorillas$nests, samplers = gorillas$boundary)

pix  <- pixels(gorillas$mesh, nx = 40, ny = 40)
pred <- predict(fit, pix, ~ mySmooth + Intercept)
pred$mean <- cut_number(pred$mean, 4) ## turns the predictor into a factor

ggplot() + 
  gg(pred)

#> "Error: Discrete value supplied to continuous scale"

There is a very easy fix: use quo_name() instead of deparse(). Luckily quo_name() is re-exported from rlang in ggplot2 so you don't even need another dependence.

BUT all that may be useless since the whole chunck of code is currently commented on the devel version, so I won't PR.

I did not check if the other calls to deparse in the code are problematic or not. ++

Alex

finnlindgren commented 3 years ago

Going through old bug reports to check if they are still applicable. It seems colour scale setting is best left to the user, since if inlabru::gg also did it, there would be warnings about duplicate scales etc. Better to let gg do one job only, so the referred code won't be used. quo_name seems to be out of favour now, referring to as_name instead, but the other deparse calls in the code seem to do their intended base-R job.