Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

default_aes not used in compute_*() methods #3860

Open
thomasp85 opened this issue Mar 6, 2020 · 10 comments · May be fixed by #6099
Open

default_aes not used in compute_*() methods #3860

thomasp85 opened this issue Mar 6, 2020 · 10 comments · May be fixed by #6099
Labels
internals 🔎

Comments

@thomasp85
Copy link
Member

While writing about extensions I noticed that the content of default_aes in a stat is not added to the data before it is passed through the compute_*() mill. This effectively renders it useless for anything but defining mappings for the geom.

The current code reflects this as it has all sorts of code around extracting non-required aesthetics from the data, e.g. in StatBinHex

ggplot2/R/stat-binhex.r

Lines 45 to 65 in 1223de2

StatBinhex <- ggproto("StatBinhex", Stat,
default_aes = aes(weight = 1, fill = after_stat(count)),
required_aes = c("x", "y"),
compute_group = function(data, scales, binwidth = NULL, bins = 30,
na.rm = FALSE) {
try_require("hexbin", "stat_binhex")
binwidth <- binwidth %||% hex_binwidth(bins, scales)
wt <- data$weight %||% rep(1L, nrow(data))
out <- hexBinSummarise(data$x, data$y, wt, binwidth, sum)
out$density <- as.vector(out$value / sum(out$value, na.rm = TRUE))
out$ndensity <- out$density / max(out$density, na.rm = TRUE)
out$count <- out$value
out$ncount <- out$count / max(out$count, na.rm = TRUE)
out$value <- NULL
out
}
)

we set a default weight, but still has to guard it with data$weight %||% rep(1L, nrow(data)) when using it in compute_group()

I cannot see any meaningful reason why this is so, and the fix seems obvious. Have I missed an underlying reason @hadley?
@thomasp85
Copy link
Member Author

Digging a bit deeper. It appears that it is also not possible to set aesthetics from a stat in the same way as it is possible to set aesthetics in a geom...

All this seems to indicate a conscious decision to let aesthetics in stat behave almost, but no quite, like the aesthetics in geom, but the reason eludes me

@hadley
Copy link
Member

hadley commented Mar 6, 2020

If there's an underlying reason I have long since forgotten it. Maybe it's to avoid some sort of precedence clash between stats and geoms?

@thomasp85
Copy link
Member Author

I'll try to see if this is fixable without any nasty side-effects... It completely caught me off-guard

@thomasp85
Copy link
Member Author

Hmm... So the sticking point (for the first issue) is that default_aes is forward facing for the stat and backwards facing for the geom, i.e. the defaults are applied after computation and before drawing. This allows the stat to set default mappings based on its own output, e.g. y = after_scale(count)...

Adding two passes of default mappings to stat could either be done by differentiating the default_aes mappings or have two different slots. It is definitely weird right now where we declare defaults that are never enforced and then sprinkle safe guards all over the code.

The other issue (setting aesthetics for stats rather than mapping them) is kind of orthogonal. The main issue I can see is if both a stat and a geom share an aesthetic which is then set by the user. What should happen in that case.

I'd like to get some input on this from @clauswilke and @yutannihilation as well

@clauswilke
Copy link
Member

I think the key issue to consider is that default_aes serves two different purposes: 1. To actually set the default value of an aes. 2. To alert the layer that the respective aes is in use and should be recognized when provided as a parameter. In stats, for any aesthetics that don't make it into the final geom, we generally use default_aes for the second purpose only. That seems like a poor design. I think it would be better to add an additional slot that can be used to declare and set defaults for aes's needed at the beginning of the stat computation.

A related issue is #3250, where the layer doesn't know which aesthetics should and should not get dropped during the computation. The approach to fixing it that I came up with is to add a slot listing the aesthetics that are dropped during computation: https://github.com/tidyverse/ggplot2/pull/3251/files

We should consider these two issues jointly.

@thomasp85
Copy link
Member Author

We have non_missing_aes for the second case already

@hadley
Copy link
Member

hadley commented Mar 6, 2020

Also related to #1516

@thomasp85
Copy link
Member Author

Wow, that’s an oldie😀

@clauswilke
Copy link
Member

We have non_missing_aes for the second case already

I don't think that's true. As far as I can tell, non_missing_aes is only used for removing NAs.

Geoms have default_aes, required_aes, and optional_aes:

ggplot2/R/geom-.r

Lines 183 to 190 in bb86d33

aesthetics = function(self) {
if (is.null(self$required_aes)) {
required_aes <- NULL
} else {
required_aes <- unlist(strsplit(self$required_aes, '|', fixed = TRUE))
}
c(union(required_aes, names(self$default_aes)), self$optional_aes, "group")
}

Stats have only default_aes and required_aes:

ggplot2/R/stat-.r

Lines 152 to 159 in bb86d33

aesthetics = function(self) {
if (is.null(self$required_aes)) {
required_aes <- NULL
} else {
required_aes <- unlist(strsplit(self$required_aes, '|', fixed = TRUE))
}
c(union(required_aes, names(self$default_aes)), "group")
}

None of these slots are documented in the code:

ggplot2/R/geom-.r

Lines 59 to 63 in bb86d33

required_aes = character(),
non_missing_aes = character(),
optional_aes = character(),
default_aes = aes(),

ggplot2/R/stat-.r

Lines 58 to 62 in bb86d33

default_aes = aes(),
required_aes = character(),
non_missing_aes = character(),

@thomasp85
Copy link
Member Author

Ah, yeah... I was thinking of optional_aes

@thomasp85 thomasp85 mentioned this issue May 15, 2020
Merged
@teunbrand teunbrand mentioned this issue Jan 26, 2023
Open
@teunbrand teunbrand linked a pull request Sep 12, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
internals 🔎
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stats use default_aes prior to computation teunbrand/ggplot2
3 participants
@hadley @thomasp85 @clauswilke