Skip to content

Don't switch scale type when using defaults #4456

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

Merged
merged 5 commits into from
May 5, 2021
Merged

Don't switch scale type when using defaults #4456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d229a87
don't switch scale type when using defaults
clauswilke Apr 28, 2021
8787306
rebuild documentation
clauswilke Apr 28, 2021
d6de9b5
add scale check
clauswilke May 3, 2021
5d6641e
type checking
clauswilke May 3, 2021
84cf4fe
unit tests for errors
clauswilke May 3, 2021
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
79 changes: 61 additions & 18 deletions R/scale-colour.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,11 @@
#' options(ggplot2.continuous.fill = tmp) # restore previous setting
#' @export
scale_colour_continuous <- function(...,
type = getOption("ggplot2.continuous.colour", default = "gradient")) {
type = getOption("ggplot2.continuous.colour")) {
type <- type %||% "gradient"

if (is.function(type)) {
type(...)
check_scale_type(type(...), "scale_colour_continuous", "colour")
} else if (identical(type, "gradient")) {
scale_colour_gradient(...)
} else if (identical(type, "viridis")) {
Expand All @@ -89,9 +91,11 @@ scale_colour_continuous <- function(...,
#' @rdname scale_colour_continuous
#' @export
scale_fill_continuous <- function(...,
type = getOption("ggplot2.continuous.fill", default = "gradient")) {
type = getOption("ggplot2.continuous.fill")) {
type <- type %||% "gradient"

if (is.function(type)) {
type(...)
check_scale_type(type(...), "scale_fill_continuous", "fill")
} else if (identical(type, "gradient")) {
scale_fill_gradient(...)
} else if (identical(type, "viridis")) {
Expand All @@ -104,29 +108,68 @@ scale_fill_continuous <- function(...,
#' @export
#' @rdname scale_colour_continuous
scale_colour_binned <- function(...,
type = getOption("ggplot2.binned.colour", default = getOption("ggplot2.continuous.colour", default = "gradient"))) {
type = getOption("ggplot2.binned.colour")) {
if (is.function(type)) {
type(...)
} else if (identical(type, "gradient")) {
scale_colour_steps(...)
} else if (identical(type, "viridis")) {
scale_colour_viridis_b(...)
check_scale_type(type(...), "scale_colour_binned", "colour")
} else {
abort("Unknown scale type")
type_fallback <- getOption("ggplot2.continuous.colour", default = "gradient")
# don't use fallback from scale_colour_continuous() if it is
# a function, since that would change the type of the color
# scale from binned to continuous
if (is.function(type_fallback)) {
type_fallback <- "gradient"
}
type <- type %||% type_fallback

if (identical(type, "gradient")) {
scale_colour_steps(...)
} else if (identical(type, "viridis")) {
scale_colour_viridis_b(...)
} else {
abort("Unknown scale type")
}
}
}

#' @export
#' @rdname scale_colour_continuous
scale_fill_binned <- function(...,
type = getOption("ggplot2.binned.fill", default = getOption("ggplot2.continuous.fill", default = "gradient"))) {
type = getOption("ggplot2.binned.fill")) {
if (is.function(type)) {
type(...)
} else if (identical(type, "gradient")) {
scale_fill_steps(...)
} else if (identical(type, "viridis")) {
scale_fill_viridis_b(...)
check_scale_type(type(...), "scale_fill_binned", "fill")
} else {
abort("Unknown scale type")
type_fallback <- getOption("ggplot2.continuous.fill", default = "gradient")
# don't use fallback from scale_colour_continuous() if it is
# a function, since that would change the type of the color
# scale from binned to continuous
if (is.function(type_fallback)) {
type_fallback <- "gradient"
}
type <- type %||% type_fallback

if (identical(type, "gradient")) {
scale_fill_steps(...)
} else if (identical(type, "viridis")) {
scale_fill_viridis_b(...)
} else {
abort("Unknown scale type")
}
}
}


# helper function to make sure that the provided scale is of the correct
# type (i.e., is continuous and works with the provided aesthetic)
check_scale_type <- function(scale, name, aesthetic) {
if (!is.ggproto(scale) || !inherits(scale, "Scale")) {
abort(glue("The `type` argument of `{name}()` must return a continuous scale for the {aesthetic} aesthetic. The provided object is not a scale function."))
}
if (!isTRUE(aesthetic %in% scale$aesthetics)) {
abort(glue("The `type` argument of `{name}()` must return a continuous scale for the {aesthetic} aesthetic. The provided scale works with the following aesthetics: {glue_collapse(scale$aesthetics, sep = ', ')}"))
}
if (isTRUE(scale$is_discrete())) {
abort(glue("The `type` argument of `{name}()` must return a continuous scale for the {aesthetic} aesthetic, but the provided scale is discrete."))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use this for the default discrete scale as well, in which case scale$is_discrete() should match some input argument?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related issue for discrete scales: #4149

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll look into that issue and see if I can fix it also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for pilling onto what was effectively a documentation change🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomasp85 Do you want an error or a warning when the scale doesn't contain the right aesthetic? @yutannihilation suggested a warning here, I have currently implemented an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is done differently for continuous and discrete scales. Continuous scales don't look at the defaults of other aesthetics. Maybe we should merge the current PR as is and defer a broader cleanup of this issue to the next major release, when we wanted to somewhat change how scales work anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge this, but I'm very tempted to also fix the discrete scales for this release. The current behaviour is a mistake I think...

At least it should be avoided when the defaults are functions and only when a palette is provided

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to be frank that would probably be more confusing than simply not inheriting across aesthetics at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the point for me: I think this requires a little more careful thinking, and I'm not sure I want to rush this. I can make arguments for either way. It's certainly confusing to have different default color palettes for colour and fill, but then it's also confusing to have a scale_colour_*() applied to fill or to trigger a "incorrect scale" error if you change the default colour scale to a different function and then want to use the fill aesthetic.

}

scale
}
22 changes: 4 additions & 18 deletions man/scale_colour_continuous.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions tests/testthat/test-scale-colour-continuous.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
context("test-scale-colour-continuous.R")

test_that("type argument is checked for proper input", {
expect_error(
scale_colour_continuous(type = function() "abc"),
"is not a scale function"
)
expect_error(
scale_fill_continuous(type = geom_point),
"is not a scale function"
)
expect_error(
scale_colour_binned(type = function(...) scale_colour_binned(aesthetics = c("fill", "point_colour"))),
"works with the following aesthetics: fill, point_colour"
)
expect_error(
scale_fill_binned(type = scale_fill_brewer),
"provided scale is discrete"
)
})