alanocallaghan / scater

Clone of the Bioconductor repository for the scater package.
https://bioconductor.org/packages/devel/bioc/html/scater.html
95 stars 40 forks source link

aes_string deprecation leads to incredibly hacky code #183

Closed alanocallaghan closed 11 months ago

alanocallaghan commented 1 year ago

Without aes_string, it's hard to use default colnames in the ggplot2 data. With aes_string we're effectively defining a standard plotting df:

p$data
                        X          Y colour_by order_by
1772071015_C02 -37.481051  0.8323347  4.397060        1
1772071017_G12 -35.362765  0.0702333  5.233381        2

so then we can have the colour aes map directly to colour_by rather than needing to actually know what it represents (eg, Snap25 expression), and the mapping is built iteratively depending on what's needed: https://github.com/Alanocallaghan/scater/blob/75154f8ecbb6e51c8fd07de2daeadc38670a4329/R/plot_central.R#L196-L216

Now conceptually I think you could do something with substitute or some other NSE function, but that seems incredibly hacky.

Instead, I'm going to use the fact that you can add a list of objects to a ggplot object, eg

ggplot(mtcars) + list(aes(x=mpg, y = cyl)) + geom_point()

works just fine, even if cyl is defined in the calling environment (not just in mtcars) because aes uses lazyeval.

Then we can return that list from .get_point_args in a separate list element, and add it to the plot before adding the point geom.

Unfortunately this relies on the point geom being the primary one, otherwise we risk overwriting the aes for other geoms.

This issue is mostly just to note down that this seems very likely to lead to problems, and wonder if there's a nice solution. However for the time being it's the best option.

lambdamoses commented 1 year ago

I also miss aes_string(). In my package Voyager, I used to have something like aes_use <- do.call(aes_string, list_aes) where list_aes is something like as.list(c(x = "values", fill = fill_by)) where the fill_by can be NULL, and the NULL's are automatically dropped here. That doesn't work with tidyeval in aes and do.call doesn't work with .data. Now my workaround with tidyeval in aes is to use something like aes(!!!syms(c(x = "values", fill = fill_by))). The !!! and syms come from rlang.

alanocallaghan commented 1 year ago

Thanks. I'm not using any of the rlang tidyeval stuff unless I am forced to, given that would be I think the third way of programmatically interacting with tidyverse I'd have learned and the rest deprecated

lambdamoses commented 1 year ago

I feel like I'm forced to here. Initially I was reluctant to add rlang as another dependency, but I used it anyway because rlang is already imported or suggested by other packages Voyager imports or suggests, so I'm not really adding another dependency. Tidyeval is hard to program with.

alanocallaghan commented 1 year ago

A slightly better solution is to combine aes with modifyList and then set class uneval on them after, that means you don't get things evaluating in the defining environment but you can slowly combine them all.

The solution I described only really works when everything's using the global aes, or you have to be super picky about what inherits aes