From 0f4a5ae831a88b75b844c901356c21fe4cf6ba0f Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Tue, 22 Aug 2023 00:36:40 +0200 Subject: [PATCH 1/4] add Ops.hms and tests for {+-*/} {integer,numeric} and {+-} Date --- .gitignore | 1 + NAMESPACE | 3 ++ R/generics.R | 20 +++++++++++ tests/testthat/test-generics.R | 65 ++++++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+) create mode 100644 R/generics.R create mode 100644 tests/testthat/test-generics.R diff --git a/.gitignore b/.gitignore index 220d4a6f..dbe67752 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ CRAN-RELEASE docs .vscode +.idea diff --git a/NAMESPACE b/NAMESPACE index c4ba4f6c..2e77f523 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,8 +1,11 @@ # Generated by roxygen2: do not edit by hand +S3method("+",Date) +S3method("-",Date) S3method("[<-",hms) S3method("[[",hms) S3method("units<-",hms) +S3method(Ops,hms) S3method(as.POSIXct,hms) S3method(as.POSIXlt,hms) S3method(as.character,hms) diff --git a/R/generics.R b/R/generics.R new file mode 100644 index 00000000..c45952bd --- /dev/null +++ b/R/generics.R @@ -0,0 +1,20 @@ +#' @export +Ops.hms <- function(e1, e2) { + if (.Generic == "+" && (inherits(e1, "Date") || inherits(e2, "Date"))) { + return(base::`+.Date`(e1, e2)) + } else if (.Generic == "-" && inherits(e1, "Date")) { + return(base::`-.Date`(e1, e2)) + } + res <- NextMethod(.Generic) + if (inherits(res, "difftime")) { + as_hms(res) + } else { + res + } +} + +#' @export +`+.Date` <- Ops.hms + +#' @export +`-.Date` <- Ops.hms diff --git a/tests/testthat/test-generics.R b/tests/testthat/test-generics.R new file mode 100644 index 00000000..75dc6c6b --- /dev/null +++ b/tests/testthat/test-generics.R @@ -0,0 +1,65 @@ +test_that("generic operations work as intended", { + # Test that all binary operations involving hms and one of Date, POSIXct, POSIXlt, difftime, hms, numeric, integer + # behave the same as the operation would with a difftime of the same length, except for difftime results, wich should + # be hms of the same value instead. + + hms_test_vec <- new_hms(c(0.0, NA_real_, 10.0)) + + # hms {+-*/} integer + expect_equal(hms_test_vec + 1L, new_hms(c(1.0, NA_real_, 11.0))) + expect_equal(hms_test_vec + (1L:3L), new_hms(c(1.0, NA_real_, 13.0))) + expect_equal(1L + hms_test_vec, new_hms(c(1.0, NA_real_, 11.0))) + expect_equal((1L:3L) + hms_test_vec, new_hms(c(1.0, NA_real_, 13.0))) + + expect_equal(hms_test_vec - 1L, new_hms(c(-1.0, NA_real_, 9.0))) + expect_equal(hms_test_vec - (1L:3L), new_hms(c(-1.0, NA_real_, 7.0))) + expect_equal(1L - hms_test_vec, new_hms(c(1.0, NA_real_, -9.0))) + expect_equal((1L:3L) - hms_test_vec, new_hms(c(1.0, NA_real_, -7.0))) + + expect_equal(hms_test_vec * 2L, new_hms(c(0.0, NA_real_, 20.0))) + expect_equal(hms_test_vec * (1L:3L), new_hms(c(0.0, NA_real_, 30.0))) + expect_equal(2L * hms_test_vec, new_hms(c(0.0, NA_real_, 20.0))) + expect_equal((1L:3L) * hms_test_vec, new_hms(c(0.0, NA_real_, 30.0))) + + expect_equal(hms_test_vec / 2L, new_hms(c(0.0, NA_real_, 5.0))) + expect_equal(hms_test_vec / c(1L, 2L, 4L), new_hms(c(0.0, NA_real_, 2.5))) + expect_error(1L / hms_test_vec, "second argument of / cannot be a \"difftime\" object") + + # hms {+-*/} numeric + expect_equal(hms_test_vec + 1.0, new_hms(c(1.0, NA_real_, 11.0))) + expect_equal(hms_test_vec + (1.0:3.0), new_hms(c(1.0, NA_real_, 13.0))) + expect_equal(1.0 + hms_test_vec, new_hms(c(1.0, NA_real_, 11.0))) + expect_equal((1.0:3.0) + hms_test_vec, new_hms(c(1.0, NA_real_, 13.0))) + + expect_equal(hms_test_vec - 1.0, new_hms(c(-1.0, NA_real_, 9.0))) + expect_equal(hms_test_vec - (1.0:3.0), new_hms(c(-1.0, NA_real_, 7.0))) + expect_equal(1.0 - hms_test_vec, new_hms(c(1.0, NA_real_, -9.0))) + expect_equal((1.0:3.0) - hms_test_vec, new_hms(c(1.0, NA_real_, -7.0))) + + expect_equal(hms_test_vec * 2.0, new_hms(c(0.0, NA_real_, 20.0))) + expect_equal(hms_test_vec * (1.0:3.0), new_hms(c(0.0, NA_real_, 30.0))) + expect_equal(2.0 * hms_test_vec, new_hms(c(0.0, NA_real_, 20.0))) + expect_equal((1.0:3.0) * hms_test_vec, new_hms(c(0.0, NA_real_, 30.0))) + + expect_equal(hms_test_vec / 2.0, new_hms(c(0.0, NA_real_, 5.0))) + expect_equal(hms_test_vec / c(1.0, 2.0, 4.0), new_hms(c(0.0, NA_real_, 2.5))) + expect_error(1.0 / hms_test_vec, "second argument of / cannot be a \"difftime\" object") + + # hms {+-} Date + # 86400.0 secs = 24 hours + hms_test_vec <- new_hms(86400.0 * c(1.0, NA_real_, 10.0)) + difftime_test_vec <- as.difftime(86400.0 * c(1.0, NA_real_, 10.0), units = "secs") + date_vec <- as.Date(c("1970-01-01", NA_character_, "2023-08-21")) + + expect_equal(hms_test_vec + date_vec, difftime_test_vec + date_vec) + expect_equal(date_vec + hms_test_vec, date_vec + difftime_test_vec) + + # Can't reproduce this warning because we forced compatibility + hms_result <- hms_test_vec - date_vec + expect_warning({ + difftime_result <- as_hms(difftime_test_vec - date_vec) + }, regexp = "Incompatible methods \\(\"Ops.difftime\", \"-.Date\") for \"-\"") + expect_equal(hms_result, difftime_result) + + expect_equal(date_vec - hms_test_vec, date_vec - difftime_test_vec) +}) From 32c65feffebffddc535f57ba896375082f8aaedc Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Tue, 22 Aug 2023 07:39:30 +0200 Subject: [PATCH 2/4] add {+-}.POSIXt and tests for {+-} POSIXct --- NAMESPACE | 2 ++ R/generics.R | 14 +++++++++ tests/testthat/helper-compare.R | 6 ---- tests/testthat/test-arith.R | 10 +++---- tests/testthat/test-generics.R | 50 ++++++++++++++++++++++++++++++--- 5 files changed, 67 insertions(+), 15 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 2e77f523..caa2a411 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,7 +1,9 @@ # Generated by roxygen2: do not edit by hand S3method("+",Date) +S3method("+",POSIXt) S3method("-",Date) +S3method("-",POSIXt) S3method("[<-",hms) S3method("[[",hms) S3method("units<-",hms) diff --git a/R/generics.R b/R/generics.R index c45952bd..ac3fa003 100644 --- a/R/generics.R +++ b/R/generics.R @@ -1,10 +1,18 @@ #' @export Ops.hms <- function(e1, e2) { + # This logic is hard-coded in R for difftime + # cf. https://github.com/wch/r-source/blob/a46559e8f728317da979be60e401899ae60086b2/src/main/eval.c#L3406-L3419 if (.Generic == "+" && (inherits(e1, "Date") || inherits(e2, "Date"))) { return(base::`+.Date`(e1, e2)) } else if (.Generic == "-" && inherits(e1, "Date")) { return(base::`-.Date`(e1, e2)) + } else if (.Generic == "+" && (inherits(e1, "POSIXt") || inherits(e2, "POSIXt"))) { + return(base::`+.POSIXt`(e1, e2)) + } else if (.Generic == "-" && inherits(e1, "POSIXt")) { + return(base::`-.POSIXt`(e1, e2)) } + + # delegate to Ops.difftime res <- NextMethod(.Generic) if (inherits(res, "difftime")) { as_hms(res) @@ -18,3 +26,9 @@ Ops.hms <- function(e1, e2) { #' @export `-.Date` <- Ops.hms + +#' @export +`+.POSIXt` <- Ops.hms + +#' @export +`-.POSIXt` <- Ops.hms diff --git a/tests/testthat/helper-compare.R b/tests/testthat/helper-compare.R index 7f339e24..5fc26c73 100644 --- a/tests/testthat/helper-compare.R +++ b/tests/testthat/helper-compare.R @@ -3,9 +3,3 @@ expect_hms_equal <- function(x, y) { expect_s3_class(y, "hms") expect_equal(as.numeric(x), as.numeric(y)) } - -expect_difftime_equal <- function(x, y) { - expect_s3_class(x, "difftime") - expect_s3_class(y, "difftime") - expect_equal(as.numeric(as_hms(x)), as.numeric(as_hms(y))) -} diff --git a/tests/testthat/test-arith.R b/tests/testthat/test-arith.R index 4c33ed67..99684551 100644 --- a/tests/testthat/test-arith.R +++ b/tests/testthat/test-arith.R @@ -11,11 +11,11 @@ test_that("arithmetics work", { expect_equal(hms(days = 1) + as.Date("2016-03-31"), as.Date("2016-04-01")) expect_equal(empty_tz(hms(hours = 1) + as.POSIXct("2016-03-31")), as.POSIXct("2016-03-31 01:00:00")) - expect_difftime_equal(hms(1) + hms(2), hms(3)) - expect_difftime_equal(hms(1) - hms(2), hms(-1)) - expect_difftime_equal(2 * hms(1), hms(2)) - expect_difftime_equal(hms(hours = 1) / 2, hms(minutes = 30)) - expect_difftime_equal(-hms(1), hms(-1)) + expect_hms_equal(hms(1) + hms(2), hms(3)) + expect_hms_equal(hms(1) - hms(2), hms(-1)) + expect_hms_equal(2 * hms(1), hms(2)) + expect_hms_equal(hms(hours = 1) / 2, hms(minutes = 30)) + expect_hms_equal(-hms(1), hms(-1)) }) test_that("component extraction work", { diff --git a/tests/testthat/test-generics.R b/tests/testthat/test-generics.R index 75dc6c6b..b96385fd 100644 --- a/tests/testthat/test-generics.R +++ b/tests/testthat/test-generics.R @@ -56,10 +56,52 @@ test_that("generic operations work as intended", { # Can't reproduce this warning because we forced compatibility hms_result <- hms_test_vec - date_vec - expect_warning({ - difftime_result <- as_hms(difftime_test_vec - date_vec) - }, regexp = "Incompatible methods \\(\"Ops.difftime\", \"-.Date\") for \"-\"") - expect_equal(hms_result, difftime_result) + expect_warning( + difftime_result <- as_hms(difftime_test_vec - date_vec), + regexp = "Incompatible methods \\(\"Ops.difftime\", \"-.Date\") for \"-\"" + ) + expect_equal(hms_result, difftime_result, info = "hms - Date not equal to difftime - Date") expect_equal(date_vec - hms_test_vec, date_vec - difftime_test_vec) + + # hms {+-} POSIXt + posixct_vec <- as.POSIXct( + c("2023-03-26 03:00:00", "2023-10-29 02:00:00", NA_character_, "1970-01-01 00:00:00"), + tz = "Europe/Berlin" + ) + + posix_difftime_result <- posixct_vec + as.difftime(1.0, units = "hours") + expect_equal( + format(posix_difftime_result, usetz = TRUE), + c("2023-03-26 04:00:00 CEST", "2023-10-29 02:00:00 CET", NA_character_, "1970-01-01 01:00:00 CET") + ) + expect_equal(posixct_vec + hms(hours = 1L), posix_difftime_result) + expect_equal(hms(hours = 1L) + posixct_vec, posix_difftime_result) + expect_equal(as.difftime(1.0, units = "hours") + posixct_vec, posix_difftime_result) + + posix_difftime_result <- posixct_vec - as.difftime(1.0, units = "hours") + expect_equal( + format(posix_difftime_result, usetz = TRUE), + c("2023-03-26 01:00:00 CET", "2023-10-29 01:00:00 CEST", NA, "1969-12-31 23:00:00 CET"), + info = "POSIXct - difftime" + ) + expect_equal(posixct_vec - hms(hours = 1L), posix_difftime_result) + + hms_result <- hms(hours = 1L) - posixct_vec + expect_warning( + difftime_result <- as.difftime(3600.0, units = "secs") - posixct_vec, + regexp = "Incompatible methods \\(\"Ops.difftime\", \"-.POSIXt\") for \"-\"" + ) + # FIXME this currently returns a different result than difftime - POSIXt. + # hms - POSIXt produces a hms + # difftime - POSIXt produces a POSIXt + expect_equal( + as.numeric(hms_result), as.numeric(difftime_result), + info = "hms - POSIXt not equal to difftime - POSIXt" + ) + expect_hms_equal(hms_result, new_hms(c(-1679788800.0, -1698534000.0, NA_real_, 7200.0))) + expect_equal( + difftime_result, + as.POSIXct(c(-1679788800.0, -1698534000.0, NA_real_, 7200.0), origin = "1970-01-01", tz = "Europe/Berlin") + ) }) From 7cbe1f8df597442f518f731774331ebe22476e1c Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Thu, 24 Aug 2023 18:57:54 +0200 Subject: [PATCH 3/4] only export for R >= 4.1 --- NAMESPACE | 13 ++++++++----- R/generics.R | 16 ++++++++-------- tests/testthat/test-generics.R | 1 + 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index caa2a411..d82655d1 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,13 +1,16 @@ # Generated by roxygen2: do not edit by hand -S3method("+",Date) -S3method("+",POSIXt) -S3method("-",Date) -S3method("-",POSIXt) + +if (getRversion() >= "4.1") { + S3method("+",Date) + S3method("+",POSIXt) + S3method("-",Date) + S3method("-",POSIXt) + S3method(Ops,hms) +} S3method("[<-",hms) S3method("[[",hms) S3method("units<-",hms) -S3method(Ops,hms) S3method(as.POSIXct,hms) S3method(as.POSIXlt,hms) S3method(as.character,hms) diff --git a/R/generics.R b/R/generics.R index ac3fa003..75c244ed 100644 --- a/R/generics.R +++ b/R/generics.R @@ -1,4 +1,11 @@ -#' @export +#' @rawNamespace +#' if (getRversion() >= "4.1") { +#' S3method("+",Date) +#' S3method("+",POSIXt) +#' S3method("-",Date) +#' S3method("-",POSIXt) +#' S3method(Ops,hms) +#' } Ops.hms <- function(e1, e2) { # This logic is hard-coded in R for difftime # cf. https://github.com/wch/r-source/blob/a46559e8f728317da979be60e401899ae60086b2/src/main/eval.c#L3406-L3419 @@ -21,14 +28,7 @@ Ops.hms <- function(e1, e2) { } } -#' @export `+.Date` <- Ops.hms - -#' @export `-.Date` <- Ops.hms - -#' @export `+.POSIXt` <- Ops.hms - -#' @export `-.POSIXt` <- Ops.hms diff --git a/tests/testthat/test-generics.R b/tests/testthat/test-generics.R index b96385fd..039bf2c9 100644 --- a/tests/testthat/test-generics.R +++ b/tests/testthat/test-generics.R @@ -1,4 +1,5 @@ test_that("generic operations work as intended", { + skip_if(getRversion() < "4.1") # Test that all binary operations involving hms and one of Date, POSIXct, POSIXlt, difftime, hms, numeric, integer # behave the same as the operation would with a difftime of the same length, except for difftime results, wich should # be hms of the same value instead. From 8759889cc254fe045898a7b3c7d0475d24f65f43 Mon Sep 17 00:00:00 2001 From: Alexander Rosenstock Date: Thu, 24 Aug 2023 19:00:52 +0200 Subject: [PATCH 4/4] revert removal of expect_difftime_equal() test helper for R < 4.1 --- tests/testthat/helper-compare.R | 6 ++++++ tests/testthat/test-arith.R | 18 +++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/tests/testthat/helper-compare.R b/tests/testthat/helper-compare.R index 5fc26c73..7f339e24 100644 --- a/tests/testthat/helper-compare.R +++ b/tests/testthat/helper-compare.R @@ -3,3 +3,9 @@ expect_hms_equal <- function(x, y) { expect_s3_class(y, "hms") expect_equal(as.numeric(x), as.numeric(y)) } + +expect_difftime_equal <- function(x, y) { + expect_s3_class(x, "difftime") + expect_s3_class(y, "difftime") + expect_equal(as.numeric(as_hms(x)), as.numeric(as_hms(y))) +} diff --git a/tests/testthat/test-arith.R b/tests/testthat/test-arith.R index 99684551..0a5f9b79 100644 --- a/tests/testthat/test-arith.R +++ b/tests/testthat/test-arith.R @@ -11,11 +11,19 @@ test_that("arithmetics work", { expect_equal(hms(days = 1) + as.Date("2016-03-31"), as.Date("2016-04-01")) expect_equal(empty_tz(hms(hours = 1) + as.POSIXct("2016-03-31")), as.POSIXct("2016-03-31 01:00:00")) - expect_hms_equal(hms(1) + hms(2), hms(3)) - expect_hms_equal(hms(1) - hms(2), hms(-1)) - expect_hms_equal(2 * hms(1), hms(2)) - expect_hms_equal(hms(hours = 1) / 2, hms(minutes = 30)) - expect_hms_equal(-hms(1), hms(-1)) + if (getRversion() < "4.1") { + expect_difftime_equal(hms(1) + hms(2), hms(3)) + expect_difftime_equal(hms(1) - hms(2), hms(-1)) + expect_difftime_equal(2 * hms(1), hms(2)) + expect_difftime_equal(hms(hours = 1) / 2, hms(minutes = 30)) + expect_difftime_equal(-hms(1), hms(-1)) + } else { + expect_hms_equal(hms(1) + hms(2), hms(3)) + expect_hms_equal(hms(1) - hms(2), hms(-1)) + expect_hms_equal(2 * hms(1), hms(2)) + expect_hms_equal(hms(hours = 1) / 2, hms(minutes = 30)) + expect_hms_equal(-hms(1), hms(-1)) + } }) test_that("component extraction work", {