stan-dev / projpred

Projection predictive variable selection
https://mc-stan.org/projpred/
Other
110 stars 25 forks source link

Missing exports of S3 methods #501

Closed fweber144 closed 3 months ago

fweber144 commented 5 months ago

Running devtools::document() with roxygen2 version 7.3.1 reports the following:

✖ formula.R:808: S3 method `formula.gamm4` needs @export or @exportS3method tag.
✖ divergence_minimizers.R:1216: S3 method `predict.gamm4` needs @export or @exportS3method tag.
✖ divergence_minimizers.R:1179: S3 method `predict.subfit` needs @export or @exportS3method tag.

(see https://github.com/stan-dev/projpred/pull/498#issuecomment-2081609937). To my knowledge, exporting these S3 methods is not strictly necessary at the moment (R <= 4.4.0), but the roxygen2 warning might indicate that this could change in the future. Already for avoiding these roxygen2 warnings, it would probably be better to export those S3 methods.

At some point in the past (1 or 2 years ago), I already tried to export all S3 methods, including the ones mentioned above. At that time, exporting formula.gamm4() and then avoiding explicit formula.gamm4() calls (i.e., replacing them with formula() calls) somehow led to an error which I could not resolve back then. Hence, it might be better to just export the S3 methods mentioned above and not replace their calls with the corresponding "generic-only" calls (this doesn't mean that replacing their calls with the corresponding "generic-only" calls shouldn't be tried again now).

fweber144 commented 3 months ago

Ah, now I understand why I got the error when replacing formula.gamm4() calls with formula() calls back then: The only occurrence of formula.gamm4() is in lines https://github.com/stan-dev/projpred/blob/25748e84b367ca469b11a279ff9773a870525f53/R/refmodel.R#L873-L877 and there, object has classes

[1] "stanreg" "glm"     "lm"      "gamm4"   "lmerMod"

Since rstanarm has rstanarm:::formula.stanreg(), code such as

# Source: Example section of `?rstanarm::stan_gamm4`.
library(projpred)
library(rstanarm)
options(mc.cores = parallel::detectCores(logical = FALSE))
dat <- mgcv::gamSim(1, n = 400, scale = 2) ## simulate 4 term additive truth
## Now add 20 level random effect`fac'...
dat$fac <- fac <- as.factor(sample(1:20, 400, replace = TRUE))
dat$y <- dat$y + model.matrix(~ fac - 1) %*% rnorm(20) * .5
br <- stan_gamm4(y ~ s(x0) + x1 + s(x2), data = dat, random = ~ (1 | fac),
                 chains = 1,
                 iter = 500, # for example speed
                 seed = 1140350788)

will cause formula() applied to br (within get_refmodel.stanreg(); called object there, see lines above) to dispatch to rstanarm:::formula.stanreg() instead of formula.gamm4(). So in the lines above, we really need to call formula.gamm4() explicitly. Now I thought about renaming formula.gamm4() to something like formula_gamm4_from_stanreg(), but since projpred declares submodels resulting from fit_gamm_callback() to have class "gamm4" (and projpred indeed has calls of formula() applied to such submodels), such a renaming is not possible.

Btw, the following code

library(gamm4)

set.seed(0) 
dat <- gamSim(1,n=400,scale=2) ## simulate 4 term additive truth
## Now add 20 level random effect `fac'...
dat$fac <- fac <- as.factor(sample(1:20,400,replace=TRUE))
dat$y <- dat$y + model.matrix(~fac-1)%*%rnorm(20)*.5

br <- gamm4(y~s(x0)+x1+s(x2),data=dat,random=~(1|fac))
class(br)

gives class

## [1] "list"

so in package gamm4, there are actually no objects of class "gamm4".

fweber144 commented 3 months ago

Resolved by 9f81bb115558a617f142d34d05f4c1a3e001ebc9.