From cd7199dcc43318690fc444913033ad74021ae867 Mon Sep 17 00:00:00 2001
From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
Date: Tue, 1 Aug 2023 20:20:29 +0200
Subject: [PATCH] Binned limits and reverse transform (#5357)

* Borrow `get_limits()` method

* Consider sort-order of limits

* Add tests

* Add news bullet
---
 NEWS.md                            |  6 +++++-
 R/scale-.R                         | 15 +++++++++++++--
 tests/testthat/test-scale-binned.R | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 096ac629a0..b62ba9eba0 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,10 @@
 # ggplot2 (development version)
 
+* Binned scales now treat `NA`s in limits the same way continuous scales do 
+  (#5355).
+
+* Binned scales work better with `trans = "reverse"` (#5355).
+
 * The `legend.text.align` and `legend.title.align` arguments in `theme()` are 
   deprecated. The `hjust` setting of the `legend.text` and `legend.title` 
   elements continues to fulfil the role of text alignment (@teunbrand, #5347).
@@ -7,7 +12,6 @@
 * Integers are once again valid input to theme arguments that expect numeric
   input (@teunbrand, #5369)
 
-
 * Nicer error messages for xlim/ylim arguments in coord-* functions
   (@92amartins, #4601, #5297).
 
diff --git a/R/scale-.R b/R/scale-.R
index d627e88498..6c6fbb1c78 100644
--- a/R/scale-.R
+++ b/R/scale-.R
@@ -1057,10 +1057,16 @@ ScaleBinned <- ggproto("ScaleBinned", Scale,
     expand_range4(self$get_limits(), expand)
   },
 
+  get_limits = function(self) {
+    ggproto_parent(ScaleContinuous, self)$get_limits()
+  },
+
   get_breaks = function(self, limits = self$get_limits()) {
     if (self$is_empty()) return(numeric())
 
     limits <- self$trans$inverse(limits)
+    is_rev <- limits[2] < limits[1]
+    limits <- sort(limits)
 
     if (is.null(self$breaks)) {
       return(NULL)
@@ -1107,7 +1113,11 @@ ScaleBinned <- ggproto("ScaleBinned", Scale,
         }
         new_limits_trans <- suppressWarnings(self$trans$transform(new_limits))
         limits[is.finite(new_limits_trans)] <- new_limits[is.finite(new_limits_trans)]
-        self$limits <- self$trans$transform(limits)
+        if (is_rev) {
+          self$limits <- rev(self$trans$transform(limits))
+        } else {
+          self$limits <- self$trans$transform(limits)
+        }
       }
     } else if (is.function(self$breaks)) {
       if ("n.breaks" %in% names(formals(environment(self$breaks)$f))) {
@@ -1124,7 +1134,8 @@ ScaleBinned <- ggproto("ScaleBinned", Scale,
     }
 
     # Breaks must be within limits
-    breaks <- breaks[breaks >= limits[1] & breaks <= limits[2]]
+    breaks <- oob_discard(breaks, sort(limits))
+
     self$breaks <- breaks
 
     self$trans$transform(breaks)
diff --git a/tests/testthat/test-scale-binned.R b/tests/testthat/test-scale-binned.R
index 5e788547db..af11caf879 100644
--- a/tests/testthat/test-scale-binned.R
+++ b/tests/testthat/test-scale-binned.R
@@ -44,6 +44,21 @@ test_that("binned limits should not compute out-of-bounds breaks", {
   ))
 })
 
+test_that("binned scales can use NAs in limits", {
+  scale <- scale_x_binned(limits = c(NA, 10))
+  scale$train(c(-20, 20))
+  expect_equal(scale$get_limits(), c(-20, 10))
+  scale <- scale_x_binned(limits = c(-10, NA))
+  scale$train(c(-20, 20))
+  expect_equal(scale$get_limits(), c(-10, 20))
+})
+
+test_that("binned scales can calculate breaks with reverse transformation", {
+  scale <- scale_x_binned(trans = "reverse")
+  scale$train(c(1, 9))
+  expect_equal(scale$get_breaks(), 8:2)
+})
+
 test_that('binned scales can calculate breaks on dates', {
 
   data <- seq(as.Date("2000-01-01"), as.Date("2020-01-01"), length.out = 100)