Skip to content
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

WIP: n.breaks are supplied to function-breaks. #5974

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

WIP: n.breaks are supplied to function-breaks. #5974

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5872bae
deal with NA/NULL breaks earlier
teunbrand Jul 3, 2024
2799a86
generalise `trans_support_nbreaks()` --> `support_nbreaks()`
teunbrand Jul 3, 2024
78ecf62
early exit on zero-range limits
teunbrand Jul 3, 2024
151b4b1
apply `n.breaks` regarless or whence breaks-function came
teunbrand Jul 3, 2024
4d0ed09
Throw `NA` breaks error in 1 place
teunbrand Jul 3, 2024
b4c5eb5
omit outdated comment
teunbrand Dec 2, 2024
6fedc16
resolve merge conflict
teunbrand Dec 2, 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
70 changes: 30 additions & 40 deletions R/scale-.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,12 @@ check_breaks_labels <- function(breaks, labels, call = NULL) {
if (is.null(breaks)) {
return(TRUE)
}
if (identical(breaks, NA)) {
cli::cli_abort(
"Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
call = call
)
}
Comment on lines +621 to +626
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 is an adjacent change that seemed like a good idea because we can now omit this check in the get_breaks() calculation. It is the reason why the unit tests had to be adapted.

if (is.null(labels)) {
return(TRUE)
}
Expand Down Expand Up @@ -717,6 +723,12 @@ ScaleContinuous <- ggproto("ScaleContinuous", Scale,
return(numeric())
}
transformation <- self$get_transformation()
breaks <- self$breaks %|W|% transformation$breaks
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 like to think of this line as the 'spirit of this PR'. We no longer need to treat function-breaks and default breaks any differently.


if (is.null(breaks)) {
return(NULL)
}

# Ensure limits don't exceed domain (#980)
domain <- suppressWarnings(transformation$transform(transformation$domain))
domain <- sort(domain)
Expand All @@ -725,40 +737,27 @@ ScaleContinuous <- ggproto("ScaleContinuous", Scale,
limits <- oob_squish(limits, domain)
}

# Limits in transformed space need to be converted back to data space
limits <- transformation$inverse(limits)

if (is.null(self$breaks)) {
return(NULL)
}

if (identical(self$breaks, NA)) {
cli::cli_abort(
"Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
call = self$call
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We catch this case early in the scale constructor


# Compute `zero_range()` in transformed space in case `limits` in data space
# don't support conversion to numeric (#5304)
if (zero_range(as.numeric(transformation$transform(limits)))) {
breaks <- limits[1]
} else if (is.waive(self$breaks)) {
if (!is.null(self$n.breaks) && trans_support_nbreaks(transformation)) {
breaks <- transformation$breaks(limits, self$n.breaks)
if (zero_range(as.numeric(limits))) {
return(limits[1])
}
teunbrand marked this conversation as resolved.
Show resolved Hide resolved

if (is.function(breaks)) {
# Limits in transformed space need to be converted back to data space
limits <- transformation$inverse(limits)
if (!is.null(self$n.breaks) && support_nbreaks(breaks)) {
breaks <- breaks(limits, n = self$n.breaks)
} else {
breaks <- breaks(limits)
if (!is.null(self$n.breaks)) {
cli::cli_warn(
"Ignoring {.arg n.breaks}. Use a {.cls transform} object that supports setting number of breaks.",
"Ignoring {.arg n.breaks}. Use a {.cls transform} object or \\
{.arg breaks} function that supports setting number of breaks",
call = self$call
)
}
breaks <- transformation$breaks(limits)
}
} else if (is.function(self$breaks)) {
breaks <- self$breaks(limits)
} else {
Comment on lines +754 to -771
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We no longer need separate logic for the default breaks (breaks function(!) from transformation) and function-breaks, as both now access the n.breaks logic.

breaks <- self$breaks
}

# Breaks in data space need to be converted back to transformed space
Expand Down Expand Up @@ -1005,13 +1004,6 @@ ScaleDiscrete <- ggproto("ScaleDiscrete", Scale,
return(NULL)
}

if (identical(self$breaks, NA)) {
cli::cli_abort(
"Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
call = self$call
)
}

Comment on lines -1016 to -1022
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also caught in constructor

if (is.waive(self$breaks)) {
breaks <- limits
} else if (is.function(self$breaks)) {
Expand Down Expand Up @@ -1229,14 +1221,9 @@ ScaleBinned <- ggproto("ScaleBinned", Scale,

if (is.null(self$breaks)) {
return(NULL)
} else if (identical(self$breaks, NA)) {
cli::cli_abort(
"Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
call = self$call
)
Comment on lines -1238 to -1242
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also caught in constructor

} else if (is.waive(self$breaks)) {
if (self$nice.breaks) {
if (!is.null(self$n.breaks) && trans_support_nbreaks(transformation)) {
if (!is.null(self$n.breaks) && support_nbreaks(transformation$breaks)) {
breaks <- transformation$breaks(limits, n = self$n.breaks)
} else {
if (!is.null(self$n.breaks)) {
Expand Down Expand Up @@ -1398,8 +1385,11 @@ check_transformation <- function(x, transformed, name, arg = NULL, call = NULL)
cli::cli_warn(msg, call = call)
}

trans_support_nbreaks <- function(trans) {
"n" %in% names(formals(trans$breaks))
support_nbreaks <- function(fun) {
if (inherits(fun, "ggproto_method")) {
fun <- environment(fun)$f
}
Comment on lines +1390 to +1392
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might need to deal with ggproto methods as self$breaks can be a function that gets wrapped by ggproto()

"n" %in% fn_fmls_names(fun)
}

allow_lambda <- function(x) {
Expand Down
38 changes: 6 additions & 32 deletions tests/testthat/test-scales-breaks-labels.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,38 +170,12 @@ test_that("suppressing breaks, minor_breask, and labels works", {

test_that("scale_breaks with explicit NA options (deprecated)", {
# NA is defunct, should throw error

# X
sxc <- scale_x_continuous(breaks = NA)
sxc$train(1:3)
expect_error(sxc$get_breaks())
expect_error(sxc$get_breaks_minor())

# Y
syc <- scale_y_continuous(breaks = NA)
syc$train(1:3)
expect_error(syc$get_breaks())
expect_error(syc$get_breaks_minor())

# Alpha
sac <- scale_alpha_continuous(breaks = NA)
sac$train(1:3)
expect_error(sac$get_breaks())

# Size
ssc <- scale_size_continuous(breaks = NA)
ssc$train(1:3)
expect_error(ssc$get_breaks())

# Fill
sfc <- scale_fill_continuous(breaks = NA)
sfc$train(1:3)
expect_error(sfc$get_breaks())

# Colour
scc <- scale_colour_continuous(breaks = NA)
scc$train(1:3)
expect_error(scc$get_breaks())
expect_error(scale_x_continuous(breaks = NA))
expect_error(scale_y_continuous(breaks = NA))
expect_error(scale_alpha_continuous(breaks = NA))
expect_error(scale_size_continuous(breaks = NA))
expect_error(scale_fill_continuous(breaks = NA))
expect_error(scale_colour_continuous(breaks = NA))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests can all be simplified as the constructor now calls out NA-breaks.

})

test_that("breaks can be specified by names of labels", {
Expand Down
9 changes: 3 additions & 6 deletions tests/testthat/test-scales.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,21 +432,18 @@ test_that("scales accept lambda notation for function input", {

test_that("breaks and labels are correctly checked", {
expect_snapshot_error(check_breaks_labels(1:10, letters))
p <- ggplot(mtcars) + geom_point(aes(mpg, disp)) + scale_x_continuous(breaks = NA)
expect_snapshot_error(ggplot_build(p))
expect_snapshot_error(scale_x_continuous(breaks = NA))
p <- ggplot(mtcars) + geom_point(aes(mpg, disp)) + scale_x_continuous(minor_breaks = NA)
expect_snapshot_error(ggplot_build(p))
p <- ggplot(mtcars) + geom_point(aes(mpg, disp)) + scale_x_continuous(labels = NA)
expect_snapshot_error(ggplotGrob(p))
p <- ggplot(mtcars) + geom_point(aes(mpg, disp)) + scale_x_continuous(labels = function(x) 1:2)
expect_snapshot_error(ggplotGrob(p))
p <- ggplot(mtcars) + geom_bar(aes(factor(gear))) + scale_x_discrete(breaks = NA)
expect_snapshot_error(ggplot_build(p))
expect_snapshot_error(scale_x_discrete(breaks = NA))
p <- ggplot(mtcars) + geom_bar(aes(factor(gear))) + scale_x_discrete(labels = NA)
expect_snapshot_error(ggplotGrob(p))

p <- ggplot(mtcars) + geom_bar(aes(mpg)) + scale_x_binned(breaks = NA)
expect_snapshot_error(ggplot_build(p))
expect_snapshot_error(scale_x_binned(breaks = NA))
Comment on lines -434 to +444
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again simplification due to breaks = NA being caught in the constructor

p <- ggplot(mtcars) + geom_bar(aes(mpg)) + scale_x_binned(labels = NA)
expect_snapshot_error(ggplotGrob(p))
p <- ggplot(mtcars) + geom_bar(aes(mpg)) + scale_x_binned(labels = function(x) 1:2)
Expand Down
Loading