Skip to content

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

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

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

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

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
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
89316f6
fix merge conflict
teunbrand Mar 26, 2025
1d44b9d
include binned scales
teunbrand Mar 26, 2025
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
85 changes: 40 additions & 45 deletions R/scale-.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,12 @@
if (is.null(breaks) || is.null(labels)) {
return(invisible())
}
if (identical(breaks, NA)) {
cli::cli_abort(
"Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
call = call
)
}
Comment on lines +643 to +648
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.


bad_labels <- is.atomic(breaks) && is.atomic(labels) &&
length(breaks) != length(labels)
Expand Down Expand Up @@ -751,48 +757,38 @@
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)
# To avoid NaN causing issues. NaN are dropped by the sort()
if (length(domain) == 2 && !zero_range(domain)) {
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
)
if (zero_range(as.numeric(limits))) {
return(limits[1])
}

# 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.waiver(self$breaks)) {
if (!is.null(self$n.breaks) && trans_support_nbreaks(transformation)) {
breaks <- transformation$breaks(limits, self$n.breaks)
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 +777 to -794
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 @@ -1046,13 +1042,6 @@
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 -1049 to -1055
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.waiver(self$breaks)) {
breaks <- limits
} else if (is.function(self$breaks)) {
Expand Down Expand Up @@ -1268,14 +1257,9 @@

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 -1271 to -1275
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.waiver(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 @@ -1332,9 +1316,16 @@
}
}
} else if (is.function(self$breaks)) {
if ("n.breaks" %in% names(formals(environment(self$breaks)$f))) {
fmls <- names(formals(environment(self$breaks)$f))
if (any(c("n", "n.breaks") %in% fmls)) {
n.breaks <- self$n.breaks %||% 5 # same default as trans objects
breaks <- self$breaks(limits, n.breaks = n.breaks)
# TODO: we should only allow `n` argument and not `n.breaks` to be
# consistent with other scales. We should start deprecation at some point.
if ("n.breaks" %in% fmls) {
breaks <- self$breaks(limits, n.breaks = n.breaks)
} else {
breaks <- self$breaks(limits, n = n.breaks)
}
} else {
if (!is.null(self$n.breaks)) {
cli::cli_warn(
Expand Down Expand Up @@ -1439,6 +1430,14 @@
cli::cli_warn(msg, call = call)
}


support_nbreaks <- function(fun) {
if (inherits(fun, "ggproto_method")) {
fun <- environment(fun)$f

Check warning on line 1436 in R/scale-.R

View check run for this annotation

Codecov / codecov/patch

R/scale-.R#L1436 

Added line #L1436 was not covered by tests
}
Comment on lines +1435 to +1437
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)
}

check_continuous_limits <- function(limits, ...,
arg = caller_arg(limits),
call = caller_env()) {
Expand All @@ -1449,10 +1448,6 @@
check_length(limits, 2L, arg = arg, call = call)
}

trans_support_nbreaks <- function(trans) {
"n" %in% names(formals(trans$breaks))
}

allow_lambda <- function(x) {
if (is_formula(x)) as_function(x) else x
}
0 ...estthat/_snaps/scale-colour-continuous.md → tests/testthat/_snaps/scale-colour.md
View file
Open in desktop
File renamed without changes.
64 changes: 0 additions & 64 deletions tests/testthat/_snaps/scales-breaks-labels.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,67 +67,3 @@
Error in `scale_x_datetime()`:
! Invalid `minor_breaks` specification. Use `NULL`, not `NA`.

# scale_breaks with explicit NA options (deprecated)

Code
sxc$get_breaks()
Condition
Error in `scale_x_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
sxc$get_breaks_minor()
Condition
Error in `scale_x_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
syc$get_breaks()
Condition
Error in `scale_y_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
syc$get_breaks_minor()
Condition
Error in `scale_y_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
sac$get_breaks()
Condition
Error in `scale_alpha_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
ssc$get_breaks()
Condition
Error in `scale_size_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
sfc$get_breaks()
Condition
Error in `scale_fill_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

---

Code
scc$get_breaks()
Condition
Error in `scale_colour_continuous()`:
! Invalid `breaks` specification. Use `NULL`, not `NA`.

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 @@ -186,38 +186,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_snapshot(sxc$get_breaks(), error = TRUE)
expect_snapshot(sxc$get_breaks_minor(), error = TRUE)

# Y
syc <- scale_y_continuous(breaks = NA)
syc$train(1:3)
expect_snapshot(syc$get_breaks(), error = TRUE)
expect_snapshot(syc$get_breaks_minor(), error = TRUE)

# Alpha
sac <- scale_alpha_continuous(breaks = NA)
sac$train(1:3)
expect_snapshot(sac$get_breaks(), error = TRUE)

# Size
ssc <- scale_size_continuous(breaks = NA)
ssc$train(1:3)
expect_snapshot(ssc$get_breaks(), error = TRUE)

# Fill
sfc <- scale_fill_continuous(breaks = NA)
sfc$train(1:3)
expect_snapshot(sfc$get_breaks(), error = TRUE)

# Colour
scc <- scale_colour_continuous(breaks = NA)
scc$train(1:3)
expect_snapshot(scc$get_breaks(), error = TRUE)
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))
})

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 @@ -430,21 +430,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