Don't attach external packages

[clauswilke]

This is work in progress, as discussed in #3126. There are a few cases where we attach external packages. Ideally, we would never attach external packages.

[clauswilke]

@hadley I think this is ready for review now. try_require() doesn't attach packages anymore. There's one special case in stat_quantile() where we do need to attach quantreg, however. I don't think it can be worked around, because the quantreg functions can't handle namespaces in the formula interface (just like mgcv).

[moodymudskipper]

Defining in the package unexported functions rqss <- quantreg::rqss and rq <- quantreg::rq is not good enough ?

[yutannihilation]

Defining in the package unexported functions

We can do it at the moment, but there's no guarantee that package will work only with these functions in future. I think we have no choice but to attach, if the package is designed to work fine only when it is attached.

[clauswilke]

Hadley, I agree, but I don't think that's currently possible. See reprex below. The rqss() function doesn't work unless quantreg is attached.

I would propose to merge the patch as is, since it's a major improvement over the current status, and then open an issue about that remaining require() and contact the quantreg maintainers to see if they can modify their package so we can use it without attaching.

Alternatively, I think we could import quantreg instead of only suggesting it, but that seems the wrong step to take for such a minor feature.

n <- 200
x <- sort(rchisq(n,4))
z <- x + rnorm(n)
y <- log(x)+ .1*(log(x))^2 + log(x)*rnorm(n)/4 + z

# doesn't work
quantreg::rqss(y ~ qss(x, constraint= "N") + z)
#> Error in qss(x, constraint = "N"): could not find function "qss"

# doesn't work
quantreg::rqss(y ~ quantreg::qss(x, constraint= "N") + z)
#> Error in model.frame.default(formula = y ~ quantreg::qss(x, constraint = "N") + : invalid type (list) for variable 'quantreg::qss(x, constraint = "N")'

# works
library(quantreg)
#> Loading required package: SparseM
#> 
#> Attaching package: 'SparseM'
#> The following object is masked from 'package:base':
#> 
#>     backsolve
rqss(y ~ qss(x, constraint= "N") + z)
#> Formula:
#> y ~ qss(x, constraint = "N") + z
#> Quantile fidelity at tau =  0.5 is 21.72566 
#> Estimated Model Dimension is 18

^{Created on 2019-02-13 by the reprex package (v0.2.1)}

[hadley]

Random idea: what if we did qss <- quantreg::qss just before fitting the model?

[hadley]

Ooops, reading the thread, I see that's what @moodymudskipper suggested — but I think it will work fine given that this I'd argue that the current behaviour (not supporting quantreq::qss()) is a buglet in their evaluation of formulas. If it works now, I don't think it is likely that changes to quantreg in the future will break it, or require importing additional functions.

[hadley]

You need to protect this block with is.character(), and can you please use https://style.tidyverse.org/syntax.html#indenting style?

[clauswilke]

Do you mean "you don't need ..."? I added is.character() in an earlier version that needed it but now it seems superfluous.

[hadley]

Yup sorry.

[hadley]

This still needs try_require() to get the nice error message

[clauswilke]

No, we just called it before entering the outer if statement:

ggplot2/R/stat-quantile.r

Lines 53 to 61 in 8d02957

	try_require("quantreg", "stat_quantile")
	if (is.null(formula)) {
	if (method == "rqss") {
	# we need to attach quantreg for qss to work inside formula
	require("quantreg")
	formula <- eval(substitute(y ~ qss(x, lambda = lambda)),
	list(lambda = lambda))
	} else {

The try_require() for MatrixModels is not needed at all, as far as I can tell, because we are not directly calling any functions from MatrixModels and quantreg imports MatrixModels.

[clauswilke]

I want to say I tried something like the qss <- quantreg::qss trick for mgcv in geom_smooth() and it didn't work then. But I'll try here and see what happens.

[clauswilke]

I just committed a version that seems to work without attaching anything. It's not entirely clear to me why. I think the reason is do.call() uses envir = parent.frame(), and that is the environment of the compute_group() function in this case.

library(ggplot2)

m <- ggplot(mpg, aes(displ, 1 / hwy)) + geom_point()
m + geom_quantile()
#> Smoothing formula not specified. Using: y ~ x

m + geom_quantile(method = "rqss")
#> Smoothing formula not specified. Using: y ~ qss(x, lambda = 1)

sessionInfo()
#> R version 3.5.0 (2018-04-23)
#> Platform: x86_64-apple-darwin15.6.0 (64-bit)
#> Running under: macOS Sierra 10.12.6
#> 
#> Matrix products: default
#> BLAS: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRblas.0.dylib
#> LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib
#> 
#> locale:
#> [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8
#> 
#> attached base packages:
#> [1] stats     graphics  grDevices utils     datasets  methods   base     
#> 
#> other attached packages:
#> [1] ggplot2_3.1.0.9000
#> 
#> loaded via a namespace (and not attached):
#>  [1] Rcpp_1.0.0         knitr_1.20         magrittr_1.5      
#>  [4] tidyselect_0.2.5   munsell_0.5.0      lattice_0.20-35   
#>  [7] colorspace_1.4-0   R6_2.3.0           rlang_0.3.1       
#> [10] stringr_1.3.1      dplyr_0.8.0.9000   tools_3.5.0       
#> [13] grid_3.5.0         gtable_0.2.0       quantreg_5.36     
#> [16] withr_2.1.2.9000   MatrixModels_0.4-1 htmltools_0.3.6   
#> [19] assertthat_0.2.0   yaml_2.2.0         lazyeval_0.2.1    
#> [22] rprojroot_1.3-2    digest_0.6.18      tibble_2.0.1      
#> [25] crayon_1.3.4       Matrix_1.2-14      purrr_0.2.5       
#> [28] glue_1.3.0         evaluate_0.11      rmarkdown_1.10    
#> [31] labeling_0.3       stringi_1.2.4      compiler_3.5.0    
#> [34] pillar_1.3.1       scales_1.0.0       backports_1.1.2   
#> [37] SparseM_1.77       pkgconfig_2.0.2

^{Created on 2019-02-14 by the reprex package (v0.2.1)}

[hadley]

Nice investigation - thanks! (Just needs news bullet prior to merge)

[yutannihilation]

I don't think it is likely that changes to quantreg in the future will break it, or require importing additional functions.

OK, agreed. Thanks.

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/