Warn about ignored scales and guides #6032
Conversation
the `guide_none()` test was testing incorrectly but getting the right answer somehow anyway
| @@ -354,15 +354,15 @@ Layer <- ggproto("Layer", NULL, | |||
|
|
|||
| compute_statistic = function(self, data, layout) { | |||
| if (empty(data)) | |||
| return(data_frame0()) | |||
| return(data) | |||
There was a problem hiding this comment.
In this file, 0-row data still contains relevant information in terms of column names, so we don't return 0-column data here.
| npscales$train(data) | ||
| plot$guides <- plot$guides$build(npscales, plot$layers, plot$labels, data) | ||
| data <- npscales$map(data, layers) |
There was a problem hiding this comment.
In order to warn about guides when there are no non-position scales, the guide building needed to be executed unconditionally. The npscales$n() == 0 condition is handled in new <ScalesList> methods.
| train_df = function(self, df, drop = FALSE) { | ||
| if (empty(df) || length(self$scales) == 0) { | ||
| return() | ||
| } | ||
| lapply(self$scales, function(scale) scale$train_df(df = df)) | ||
| }, | ||
|
|
||
| map = function(self, data, layers) { |
There was a problem hiding this comment.
I'm not really a fan of this as layers and scales should ideally be separate on only interact through the data, but I haven't found a better way around this.
|
|
||
| known <- unique(unlist(c( | ||
| lapply(data, colnames), | ||
| lapply(layers, function(x) names(x$computed_mapping)) |
There was a problem hiding this comment.
Sometimes you can have a mapping like colour = after_scale(alpha(fill, 0.3)) that isn't instantiated as a column in the data yet. In order to not warn about these, we need to access the layer's mapping.
| built <- ggplot_build(p) | ||
| plot <- built$plot | ||
| guides <- guides_list(plot$guides) | ||
| guides <- guides$build( | ||
| plot$scales, | ||
| plot$layers, | ||
| plot$labels | ||
| ) | ||
| expect_length(ggplot_build(p)$plot$guides$guides, 0) | ||
|
|
||
| p <- ggplot(mpg, aes(cty, hwy, colour = class)) + | ||
| geom_point() + | ||
| guides(colour = guide_none()) | ||
|
|
||
| expect_length(guides$guides, 0) |
There was a problem hiding this comment.
This test wasn't testing what is was supposed to be testing.
| guides(size = guide_legend(order = 2), | ||
| color = guide_colorbar(order = 1)) + |
There was a problem hiding this comment.
This plot wasn't using the shape aesthetic, so the test was wrong. The checks in this PR correctly picked up on this.
|
This seems surprisingly complicated. Isn't there a point in the build process where all mapped aesthetics are known and we can check that list against the provided scales and guides? |
|
Agree that it is more complicated than one'd hope. The reason that the approach you outline doesn't currently work is because |
|
I'm giving up on this, this is too complicated to be worth it. |
This PR aims to fix #3743 and guides indentified in #3196 (comment).
Briefly, when using a scale or guide for an aesthetic that is not in use, you'll get a warning.
Implementation-wise, it wasn't too straightforward to allow guides and scales to assess their usage. A few remarks on this in the code.
An example:
Created on 2024-08-07 with reprex v2.1.1