-
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
Changes from all commits
bf1ffd8
4e741de
1f79c8d
bafc6e1
6d9ac97
55c078e
fbba60c
3e4a9ce
4c1e587
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,14 +102,9 @@ ggplot_build.ggplot <- function(plot) { | |
|
||
# Train and map non-position scales and guides | ||
npscales <- scales$non_position_scales() | ||
if (npscales$n() > 0) { | ||
lapply(data, npscales$train_df) | ||
plot$guides <- plot$guides$build(npscales, plot$layers, plot$labels, data) | ||
data <- lapply(data, npscales$map_df) | ||
} else { | ||
# Only keep custom guides if there are no non-position scales | ||
plot$guides <- plot$guides$get_custom() | ||
} | ||
npscales$train(data) | ||
plot$guides <- plot$guides$build(npscales, plot$layers, plot$labels, data) | ||
data <- npscales$map(data, layers) | ||
Comment on lines
+105
to
+107
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
data <- .expose_data(data) | ||
|
||
# Fill in defaults etc. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,13 +61,44 @@ ScalesList <- ggproto("ScalesList", NULL, | |
scale[[1]] | ||
}, | ||
|
||
train = function(self, data, drop = FALSE) { | ||
if (length(self$scales) == 0) { | ||
return() | ||
} | ||
lapply(data, self$train_df, drop = drop) | ||
}, | ||
|
||
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 commentThe 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. |
||
if (length(self$scales) == 0) { | ||
return(data) | ||
} | ||
aesthetics <- lapply(self$scales, `[[`, "aesthetics") | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes you can have a mapping like |
||
))) | ||
|
||
known <- unlist(aesthetics) %in% known | ||
known <- vapply(vec_chop(known, sizes = lengths(aesthetics)), any, logical(1)) | ||
|
||
unknown <- unlist(aesthetics[!known]) | ||
if (length(unknown) > 0) { | ||
cli::cli_warn( | ||
"Ignoring scale{?s} for unused aesthetics: {.val {unknown}}." | ||
) | ||
} | ||
|
||
lapply(data, self$map_df) | ||
}, | ||
|
||
map_df = function(self, df) { | ||
if (empty(df) || length(self$scales) == 0) { | ||
return(df) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,11 @@ test_that("axis_label_overlap_priority always returns the correct number of elem | |
expect_setequal(axis_label_priority(100), seq_len(100)) | ||
}) | ||
|
||
test_that("a warning is generated when declaring unused guides", { | ||
plot <- ggplot(mtcars, aes(disp, mpg)) + geom_point() + guides(colour = "legend") | ||
expect_warning(ggplot_build(plot), "Ignoring unknown guide") | ||
}) | ||
|
||
test_that("a warning is generated when guides are drawn at a location that doesn't make sense", { | ||
plot <- ggplot(mpg, aes(class, hwy)) + | ||
geom_point() + | ||
|
@@ -137,16 +142,13 @@ test_that("guide_none() can be used in non-position scales", { | |
geom_point() + | ||
scale_color_discrete(guide = guide_none()) | ||
|
||
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) | ||
Comment on lines
-140
to
-149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test wasn't testing what is was supposed to be testing. |
||
expect_length(ggplot_build(p)$plot$guides$guides, 0) | ||
}) | ||
|
||
test_that("Using non-position guides for position scales results in an informative error", { | ||
|
@@ -562,7 +564,12 @@ test_that("legends can be forced to display unrelated geoms", { | |
limits = c("A", "B") | ||
) | ||
|
||
b <- ggplot_build(p) | ||
# Should complain about useless scale, as it doesn't map anything | ||
expect_warning( | ||
b <- ggplot_build(p), | ||
"Ignoring scale" | ||
) | ||
|
||
legend <- b$plot$guides$params[[1]] | ||
|
||
expect_equal( | ||
|
@@ -944,8 +951,8 @@ test_that("guides title and text are positioned correctly", { | |
geom_point() + | ||
# setting the order explicitly removes the risk for failed doppelgangers | ||
# due to legends switching order | ||
guides(shape = guide_legend(order = 1), | ||
color = guide_colorbar(order = 2)) + | ||
guides(size = guide_legend(order = 2), | ||
color = guide_colorbar(order = 1)) + | ||
Comment on lines
+954
to
+955
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This plot wasn't using the |
||
theme_test() | ||
|
||
expect_doppelganger("guide title and text positioning and alignment via themes", | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.