Skip to content

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.

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

Closed
wants to merge 9 commits into from
Closed

Warn about ignored scales and guides #6032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bf1ffd8
add `ScalesList` train and map methods
teunbrand Aug 6, 2024
4e741de
unconditionally call train/map methods
teunbrand Aug 6, 2024
1f79c8d
warn about unknown and ignored guides
teunbrand Aug 7, 2024
bafc6e1
adapt tests
teunbrand Aug 7, 2024
6d9ac97
add test
teunbrand Aug 7, 2024
55c078e
layers return empty data as-is
teunbrand Aug 7, 2024
fbba60c
emit unused scale warning
teunbrand Aug 7, 2024
3e4a9ce
Also use layer's `computed_mapping` to inform valid scales
teunbrand Aug 7, 2024
4c1e587
tweak test
teunbrand Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions R/guides-.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,16 +292,27 @@ Guides <- ggproto(
no_guides <- custom

# Extract the non-position scales
scales <- scales$non_position_scales()$scales
if (length(scales) == 0) {
return(no_guides)
}
scales <- scales$scales

# Ensure a 1:1 mapping between aesthetics and scales
aesthetics <- lapply(scales, `[[`, "aesthetics")
scales <- rep.int(scales, lengths(aesthetics))
aesthetics <- unlist(aesthetics, recursive = FALSE, use.names = FALSE)

extra_guides <- setdiff(
names(self$guides),
c(aesthetics, "custom",
"x", "y", "x.sec", "y.sec",
"r", "theta", "r.sec", "theta.sec")
)
if (length(extra_guides) > 0) {
cli::cli_warn("Ignoring unknown guide{?s}: {.val {extra_guides}}.")
}

if (length(scales) < 1) {
return(no_guides)
}

# Setup and train scales
guides <- self$setup(scales, aesthetics = aesthetics)
guides$train(scales, labels)
Expand Down
8 changes: 4 additions & 4 deletions R/layer.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,15 +354,15 @@ Layer <- ggproto("Layer", NULL,

compute_statistic = function(self, data, layout) {
if (empty(data))
return(data_frame0())
return(data)
Copy link
Collaborator Author

@teunbrand teunbrand Aug 7, 2024 •
edited
Loading

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.


self$computed_stat_params <- self$stat$setup_params(data, self$stat_params)
data <- self$stat$setup_data(data, self$computed_stat_params)
self$stat$compute_layer(data, self$computed_stat_params, layout)
},

map_statistic = function(self, data, plot) {
if (empty(data)) return(data_frame0())
if (empty(data)) return(data)

# Make sure data columns are converted to correct names. If not done, a
# column with e.g. a color name will not be found in an after_stat()
Expand Down Expand Up @@ -418,7 +418,7 @@ Layer <- ggproto("Layer", NULL,
},

compute_geom_1 = function(self, data) {
if (empty(data)) return(data_frame0())
if (empty(data)) return(data)

check_required_aesthetics(
self$geom$required_aes,
Expand All @@ -430,7 +430,7 @@ Layer <- ggproto("Layer", NULL,
},

compute_position = function(self, data, layout) {
if (empty(data)) return(data_frame0())
if (empty(data)) return(data)

params <- self$position$setup_params(data)
data <- self$position$setup_data(data, params)
Expand Down
11 changes: 3 additions & 8 deletions R/plot-build.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

@teunbrand teunbrand Aug 7, 2024 •
edited
Loading

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.

data <- .expose_data(data)

# Fill in defaults etc.
Expand Down
31 changes: 31 additions & 0 deletions R/scales-.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator Author

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.

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))
Copy link
Collaborator Author

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.

)))

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)
Expand Down
31 changes: 19 additions & 12 deletions tests/testthat/test-guides.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() +
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

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.

expect_length(ggplot_build(p)$plot$guides$guides, 0)
})

test_that("Using non-position guides for position scales results in an informative error", {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

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.

theme_test()

expect_doppelganger("guide title and text positioning and alignment via themes",
Expand Down
Loading