From 6c2b593e11f63ab18d6e5dbadaa57637e1bbb966 Mon Sep 17 00:00:00 2001
From: Bill Denney <billdenney@users.noreply.github.com>
Date: Sun, 18 Dec 2022 11:42:40 -0500
Subject: [PATCH] Always allow division of identical units (#335)

* Allow division whenever input units are identical

* Ensure that changes work when one of the inputs is not a units object

* Fix code review issues

* Always allow inverse unit multiplication

* Simplify inverse-unit multiplication and same-unit division

* only call inherits() once per function call
---
 .gitignore                  |  1 +
 NEWS.md                     |  3 +++
 R/arith.R                   | 37 +++++++++++++++++++++++++++++--------
 tests/testthat/test_arith.R | 23 +++++++++++++++++++++++
 4 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/.gitignore b/.gitignore
index a444c6fe..28b2a667 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,3 +36,4 @@ configure.scan
 *.app
 # OSX-specific
 .DS_Store
+inst/doc
diff --git a/NEWS.md b/NEWS.md
index f7fb0d91..d964f35b 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -2,6 +2,9 @@
 
 * Names are preserved when doing unit conversions; #305 @billdenney
 
+* Identical units will always divide (`/`) and allow integer division (`%/%`).
+  And, inverse units will always be able to multiply; #310 @billdenney
+
 # version 0.8-1
 
 * fix `%/%` and `%%` if arguments have different units; #313
diff --git a/R/arith.R b/R/arith.R
index c2933def..3356cc6f 100644
--- a/R/arith.R
+++ b/R/arith.R
@@ -46,21 +46,41 @@ Ops.units <- function(e1, e2) {
     return(NextMethod())
 
   eq  <- .Generic %in% c("+", "-", "==", "!=", "<", ">", "<=", ">=") # requiring identical units
+  div <- .Generic %in% c("/", "%/%")                                 # division-type
+  mul <- .Generic %in% "*"                                           # multiplication-only
   prd <- .Generic %in% c("*", "/", "%/%", "%%")                      # product-type
-  pw  <- .Generic %in% c( "**", "^")                                 # power-type
+  pw  <- .Generic %in% c("**", "^")                                  # power-type
   mod <- .Generic %in% c("%/%", "%%")                                # modulo-type
   pm  <- .Generic %in% c("+", "-")                                   # addition-type
 
   if (! any(eq, prd, pw, mod))
     stop(paste("operation", .Generic, "not allowed"))
 
+  e1_inherits_units <- inherits(e1, "units")
+  e2_inherits_units <- inherits(e2, "units")
+  both_inherit_units <- e1_inherits_units && e2_inherits_units
   if (eq) {
-    if (!(inherits(e1, "units") && inherits(e2, "units")))
+    if (!(both_inherit_units))
       stop("both operands of the expression should be \"units\" objects") # nocov
     units(e2) <- units(e1) # convert before we can compare; errors if unconvertible
   }
 
-  if (mod) {
+  identical_units <-
+    both_inherit_units &&
+    identical(units(e1), units(e2))
+
+  inverse_units <-
+    both_inherit_units &&
+    identical(units(e1)$numerator, units(e2)$denominator) &&
+    identical(units(e1)$denominator, units(e2)$numerator)
+
+  if ((div && identical_units) || (mul && inverse_units)) {
+    # Special cases for identical unit division and inverse unit multiplication
+    # which may not be otherwise divisible by udunits (see #310)
+    e1 <- drop_units(e1)
+    e2 <- drop_units(e2)
+    return(set_units(NextMethod(), 1))
+  } else if (mod) {
     div <- e1 / e2
     int <- round(div)
     if (.Generic == "%/%") {
@@ -69,10 +89,10 @@ Ops.units <- function(e1, e2) {
       return(e1 - int*e2)
     }
   } else if (prd) {
-    if (!inherits(e1, "units"))
+    if (!e1_inherits_units)
       e1 <- set_units(e1, 1) # TODO: or warn?
 
-    if (!inherits(e2, "units"))
+    if (!e2_inherits_units)
       e2 <- set_units(e2, 1) # TODO: or warn?
 
     ve1 <- unclass(e1) ; ue1 <- units(e1)
@@ -88,11 +108,12 @@ Ops.units <- function(e1, e2) {
     return(.simplify_units(NextMethod(), .symbolic_units(numerator, denominator)))
 
   } else if (pw) {
-    if (inherits(e2, "units")) {
-      if (inherits(e1, "units") && identical(units(e1), units(as_units(1))))
+    if (e2_inherits_units) {
+      if (e1_inherits_units && identical(units(e1), units(as_units(1)))) {
         e1 <- drop_units(e1)
-      if (inherits(e1, "units"))
+      } else if (e1_inherits_units) {
         stop("power operation only allowed with numeric power")
+      }
 
       # code to manage things like exp(log(...)) and 10^log10(...) follows
       # this is not supported in udunits2, so we are on our own
diff --git a/tests/testthat/test_arith.R b/tests/testthat/test_arith.R
index b76ef228..6120df7c 100644
--- a/tests/testthat/test_arith.R
+++ b/tests/testthat/test_arith.R
@@ -195,3 +195,26 @@ test_that("we obtain mixed units when taking powers of multiple integers", {
   p = 4:1
   expect_equal(a ^ p, c(set_units(1, m^4), set_units(8, m^3), set_units(9, m^2), set_units(4, m),  allow_mixed=TRUE))
 })
+
+test_that("identical units can always be divided and return unitless (#310)", {
+  x1 <- log(set_units(100, "g"))
+  x2 <- log(set_units(4, "g"))
+  expect_equal(
+    x1/x2,
+    set_units(log(100)/log(4), "")
+  )
+  expect_equal(
+    x1 %/% x2,
+    set_units(log(100) %/% log(4), "")
+  )
+})
+
+test_that("inverse units can always be multiplied and return unitless (related to #310)", {
+  x1 <- log(set_units(100, "g"))
+  x2 <- log(set_units(4, "g"))
+  expect_equal(
+    x1*(1/x2),
+    set_units(log(100)/log(4), "")
+  )
+})
+