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

teunbrand · 2024-07-03T18:54:24Z

This PR aims to fix #5972.

Briefly, it ensures that function-breaks are called with the n = n.breaks argument when supported, not just breaks derived from the transformation.

Example:

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

ggplot(mpg, aes(displ, hwy)) +
  geom_point() +
  scale_y_continuous(
    breaks = scales::breaks_extended(),
    n.breaks = 20
  )

^{Created on 2024-07-03 with reprex v2.1.0}

I consider this PR a WIP because I'd also like to treat binned scales, e.g. #5972 (comment). There are a few things that complicate this.

First, this has sort-of already been done in #3575, except instead of n, the n.breaks argument is used.

ggplot2/R/scale-.R

Lines 1287 to 1291 in 2610840

    
           } else if (is.function(self$breaks)) { 
        
             if ("n.breaks" %in% names(formals(environment(self$breaks)$f))) { 
        
               n.breaks <- self$n.breaks %||% 5 # same default as trans objects 
        
               breaks <- self$breaks(limits, n.breaks = n.breaks) 
        
             } else {

I'm not sure how widespread an n.breaks argument is, but ideally we should only support n or n.breaks and not both. However, for backwards compatibility reasons, we might have to support both.

Secondly, scale_x/y_binned() already have a default n.breaks argument:

ggplot2/R/scale-binned.R

Line 28 in 2610840

scale_x_binned <- function(name = waiver(), n.breaks = 10, nice.breaks = TRUE,

This would interfere with the strategy, because it would override n when providing breaks = scales::extended_breaks(n = 20) so that the internal function is called with n = 10. We could amend this by having n.breaks = 10 be an internal default for transformation$breaks() only.

In any case, I'd be happy to be advised on the case of binned scales.

teunbrand · 2024-07-03T18:58:26Z

R/scale-.R

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


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.

thomasp85

You are deleting an awful lot of logic without adding in an equivalent amount... can you walk me through it?

R/scale-.R

teunbrand · 2024-08-26T11:27:59Z

R/scale-.R

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


We catch this case early in the scale constructor

teunbrand · 2024-08-26T11:30:35Z

R/scale-.R

+    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 {


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.

teunbrand · 2024-08-26T11:31:13Z

R/scale-.R

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


Also caught in constructor

teunbrand · 2024-08-26T11:31:32Z

R/scale-.R

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


Also caught in constructor

teunbrand · 2024-08-26T11:32:11Z

R/scale-.R

+  if (inherits(fun, "ggproto_method")) {
+    fun <- environment(fun)$f
+  }


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

teunbrand · 2024-08-26T11:32:52Z

tests/testthat/test-scales-breaks-labels.R

-
-  # 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))


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

teunbrand · 2024-08-26T11:33:23Z

tests/testthat/test-scales.R

-  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))


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

teunbrand · 2024-08-26T11:34:04Z

R/scale-.R

@@ -717,6 +723,12 @@ ScaleContinuous <- ggproto("ScaleContinuous", Scale,
      return(numeric())
    }
    transformation <- self$get_transformation()
+    breaks <- self$breaks %|W|% transformation$breaks


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.

teunbrand · 2024-08-26T11:37:05Z

I tried to elaborate somewhat more in the comments above. The TL;DR: we merge in the 'breaks are a function'-branch with 'breaks come from the transformation breaks function'-branch, simplifying the logic as we only need to deal with function-breaks once.

Merge branch 'main' into n_breaks_for_all # Conflicts: # R/scale-.R # tests/testthat/test-scales-breaks-labels.R

teunbrand added 5 commits July 3, 2024 15:07

deal with NA/NULL breaks earlier

5872bae

generalise trans_support_nbreaks() --> support_nbreaks()

2799a86

early exit on zero-range limits

78ecf62

apply n.breaks regarless or whence breaks-function came

151b4b1

Throw NA breaks error in 1 place

4d0ed09

teunbrand commented Jul 3, 2024

View reviewed changes

thomasp85 reviewed Aug 22, 2024

View reviewed changes

teunbrand commented Aug 26, 2024

View reviewed changes

R/scale-.R Outdated Show resolved Hide resolved

teunbrand commented Aug 26, 2024

View reviewed changes

teunbrand mentioned this pull request Oct 18, 2024

Fix binned scaled not accepting function-limits if there are transformations #6145

Merged

teunbrand added 2 commits December 2, 2024 11:23

omit outdated comment

b4c5eb5

resolve merge conflict

6fedc16

Merge branch 'main' into n_breaks_for_all # Conflicts: # R/scale-.R # tests/testthat/test-scales-breaks-labels.R

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

teunbrand commented Jul 3, 2024

teunbrand Jul 3, 2024

thomasp85 left a comment

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand Aug 26, 2024

teunbrand commented Aug 26, 2024

	} else if (is.function(self$breaks)) {
	if ("n.breaks" %in% names(formals(environment(self$breaks)$f))) {
	n.breaks <- self$n.breaks %\|\|% 5 # same default as trans objects
	breaks <- self$breaks(limits, n.breaks = n.breaks)
	} else {

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

Are you sure you want to change the base?

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

Conversation

teunbrand commented Jul 3, 2024

Choose a reason for hiding this comment

thomasp85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teunbrand commented Aug 26, 2024

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

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