-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Warn about ignored scales and guides #6032
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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