`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

      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

Merge branch 'main' into n_breaks_for_all # Conflicts: # R/scale-.R

teunbrand · 2025-03-26T11:00:43Z

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

This is now solved by allowing both forms, which makes this PR ready for review.

thomasp85

LGTM

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

fix merge conflict

89316f6

Merge branch 'main' into n_breaks_for_all # Conflicts: # R/scale-.R

include binned scales

1d44b9d

teunbrand changed the title ~~WIP: n.breaks are supplied to function-breaks.~~ n.breaks are supplied to function-breaks. Mar 26, 2025

Merge branch 'main' into n_breaks_for_all

047fd51

teunbrand added this to the ggplot2 4.0.0 milestone May 6, 2025

thomasp85 approved these changes May 19, 2025

View reviewed changes

teunbrand added 2 commits May 19, 2025 09:39

approve pattern snapshots (re: tidyverse#6453)

552d41a

add news bullet

6c37068

teunbrand merged commit 3d3b425 into tidyverse:main May 19, 2025
13 checks passed

teunbrand mentioned this pull request May 19, 2025

Update svglite-based snapshots #6453

Closed

teunbrand deleted the n_breaks_for_all branch May 19, 2025 08:05

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

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

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

Uh oh!

Conversation

teunbrand commented Jul 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasp85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teunbrand commented Aug 26, 2024

Uh oh!

teunbrand commented Mar 26, 2025

Uh oh!

thomasp85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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