From 2e5e6acaa4c6b3e8c671efa0c3ece42ea21f0098 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 9 Oct 2023 11:03:26 -0400 Subject: [PATCH 1/2] Update complex tests --- tests/testthat/helper-vctrs.R | 12 +++++++++ tests/testthat/test-slice-assign.R | 4 +-- tests/testthat/test-slice.R | 2 +- tests/testthat/test-type-bare.R | 42 +++++++++++++++++++++++------- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/tests/testthat/helper-vctrs.R b/tests/testthat/helper-vctrs.R index 98f8fe793..3c013c6de 100644 --- a/tests/testthat/helper-vctrs.R +++ b/tests/testthat/helper-vctrs.R @@ -56,3 +56,15 @@ expect_equal <- function(object, expected, ..., raw2 <- function(...) { as.raw(list_unchop(list2(...), ptype = integer())) } +cpl2 <- function(...) { + # R 4.4.0 changed `as.complex(NA_real/integer/logical)` so that it always uses + # a `0` in the imaginary slot. While this is reasonable, it is annoying for + # comparison purposes in tests, where we typically propagate the `NA`. As of + # rlang 1.1.1, `cpl()` inherits this behavior change so we have a custom version + # here that works the same on all R versions. + # https://github.com/wch/r-source/commit/1a2aea9ac3c216fea718f33f712764afc34f6ee8 + out <- list2(...) + out <- as.complex(out) + out[is.na(out)] <- complex(real = NA_real_, imaginary = NA_real_) + out +} diff --git a/tests/testthat/test-slice-assign.R b/tests/testthat/test-slice-assign.R index d2c56ecfd..86fe6766e 100644 --- a/tests/testthat/test-slice-assign.R +++ b/tests/testthat/test-slice-assign.R @@ -681,7 +681,7 @@ test_that("can assign shaped base vectors with compact seqs", { expect_identical(vec_assign_seq(mat(lgl(1, 0, 1)), NA, start, size, increasing), mat(lgl(1, NA, NA))) expect_identical(vec_assign_seq(mat(int(1, 2, 3)), NA, start, size, increasing), mat(int(1, NA, NA))) expect_identical(vec_assign_seq(mat(dbl(1, 2, 3)), NA, start, size, increasing), mat(dbl(1, NA, NA))) - expect_identical(vec_assign_seq(mat(cpl(1, 2, 3)), NA, start, size, increasing), mat(cpl(1, NA, NA))) + expect_identical(vec_assign_seq(mat(cpl2(1, 2, 3)), NA, start, size, increasing), mat(cpl2(1, NA, NA))) expect_identical(vec_assign_seq(mat(chr("1", "2", "3")), NA, start, size, increasing), mat(chr("1", NA, NA))) expect_identical(vec_assign_seq(mat(raw2(1, 2, 3)), raw2(1), start, size, increasing), mat(raw2(1, 1, 1))) expect_identical(vec_assign_seq(mat(list(1, 2, 3)), NA, start, size, increasing), mat(list(1, NULL, NULL))) @@ -695,7 +695,7 @@ test_that("can assign shaped base vectors with decreasing compact seqs", { expect_identical(vec_assign_seq(mat(lgl(1, 0, 1)), NA, start, size, increasing), mat(lgl(1, NA, NA))) expect_identical(vec_assign_seq(mat(int(1, 2, 3)), NA, start, size, increasing), mat(int(1, NA, NA))) expect_identical(vec_assign_seq(mat(dbl(1, 2, 3)), NA, start, size, increasing), mat(dbl(1, NA, NA))) - expect_identical(vec_assign_seq(mat(cpl(1, 2, 3)), NA, start, size, increasing), mat(cpl(1, NA, NA))) + expect_identical(vec_assign_seq(mat(cpl2(1, 2, 3)), NA, start, size, increasing), mat(cpl2(1, NA, NA))) expect_identical(vec_assign_seq(mat(chr("1", "2", "3")), NA, start, size, increasing), mat(chr("1", NA, NA))) expect_identical(vec_assign_seq(mat(raw2(1, 2, 3)), raw2(1), start, size, increasing), mat(raw2(1, 1, 1))) expect_identical(vec_assign_seq(mat(list(1, 2, 3)), NA, start, size, increasing), mat(list(1, NULL, NULL))) diff --git a/tests/testthat/test-slice.R b/tests/testthat/test-slice.R index 3d8fe076f..e397be9dc 100644 --- a/tests/testthat/test-slice.R +++ b/tests/testthat/test-slice.R @@ -38,7 +38,7 @@ test_that("can subset with missing indices", { expect_identical(vec_slice(lgl(1, 0, 1), i), lgl(0, NA)) expect_identical(vec_slice(int(1, 2, 3), i), int(2, NA)) expect_identical(vec_slice(dbl(1, 2, 3), i), dbl(2, NA)) - expect_identical(vec_slice(cpl(1, 2, 3), i), cpl(2, NA)) + expect_identical(vec_slice(cpl2(1, 2, 3), i), cpl2(2, NA)) expect_identical(vec_slice(chr("1", "2", "3"), i), c("2", NA)) expect_identical(vec_slice(raw2(1, 2, 3), i), raw2(2, 0)) expect_identical(vec_slice(list(1, 2, 3), i), list(2, NULL)) diff --git a/tests/testthat/test-type-bare.R b/tests/testthat/test-type-bare.R index 64613b554..a1f8913b2 100644 --- a/tests/testthat/test-type-bare.R +++ b/tests/testthat/test-type-bare.R @@ -243,13 +243,27 @@ test_that("safe casts to complex works", { }) test_that("NA casts work as expected", { + # This goes through a special path for expect_equal(vec_cast(lgl(NA), cpl()), NA_complex_) - expect_equal(vec_cast(int(NA), cpl()), NA_complex_) # TODO: Use our own cast routines here? - # `as.complex(NA_real_)` and `Rf_CoerceVector(NA_real_)` coerce to - # `complex(real = NA_real_, imaginary = 0)` for some reason, but this may - # change in the future https://stat.ethz.ch/pipermail/r-devel/2023-April/082545.html + # It isn't great that this logical `NA` cast returns a different `NA` + # than the one above with just `lgl(NA)` (which is seen as unspecified). i.e. + # check the `Im()` slot between the two in R >=4.4.0. We can fix this with our + # own cast routines rather than using `vec_coerce_bare()`. + expect_type(vec_cast(lgl(NA, TRUE), cpl()), "complex") + expect_identical(is.na(vec_cast(lgl(NA, TRUE), cpl())), c(TRUE, FALSE)) + + # TODO: Use our own cast routines here? + # `as.complex(NA/NA_real_/NA_integer_)` and `Rf_CoerceVector(NA/NA_real_/NA_integer_)` + # have gone back and forth about what they return in the `Im()` slot. In some + # R versions they return `0` and in others they return `NA_real_`. + # https://stat.ethz.ch/pipermail/r-devel/2023-April/082545.html + # https://stat.ethz.ch/pipermail/r-devel/2023-September/082864.html + # expect_equal(vec_cast(int(NA), cpl()), NA_complex_) + expect_type(vec_cast(int(NA), cpl()), "complex") + expect_identical(is.na(vec_cast(int(NA), cpl())), TRUE) + # expect_equal(vec_cast(dbl(NA), cpl()), NA_complex_) expect_type(vec_cast(dbl(NA), cpl()), "complex") expect_identical(is.na(vec_cast(dbl(NA), cpl())), TRUE) @@ -263,13 +277,21 @@ test_that("Shaped NA casts work as expected", { exp_mat <- mat(NA_complex_) to_mat <- matrix(cpl()) - expect_equal(vec_cast(mat(lgl(NA)), to_mat), exp_mat) - expect_equal(vec_cast(mat(int(NA)), to_mat), exp_mat) - # TODO: Use our own cast routines here? - # `as.complex(NA_real_)` and `Rf_CoerceVector(NA_real_)` coerce to - # `complex(real = NA_real_, imaginary = 0)` for some reason, but this may - # change in the future https://stat.ethz.ch/pipermail/r-devel/2023-April/082545.html + # `as.complex(NA/NA_real_/NA_integer_)` and `Rf_CoerceVector(NA/NA_real_/NA_integer_)` + # have gone back and forth about what they return in the `Im()` slot. In some + # R versions they return `0` and in others they return `NA_real_`. + # https://stat.ethz.ch/pipermail/r-devel/2023-April/082545.html + # https://stat.ethz.ch/pipermail/r-devel/2023-September/082864.html + + # expect_equal(vec_cast(mat(lgl(NA)), to_mat), exp_mat) + expect_type(vec_cast(mat(lgl(NA)), to_mat), "complex") + expect_identical(is.na(vec_cast(mat(lgl(NA)), to_mat)), matrix(TRUE)) + + # expect_equal(vec_cast(mat(int(NA)), to_mat), exp_mat) + expect_type(vec_cast(mat(int(NA)), to_mat), "complex") + expect_identical(is.na(vec_cast(mat(int(NA)), to_mat)), matrix(TRUE)) + # expect_equal(vec_cast(mat(dbl(NA)), to_mat), exp_mat) expect_type(vec_cast(mat(dbl(NA)), to_mat), "complex") expect_identical(is.na(vec_cast(mat(dbl(NA)), to_mat)), matrix(TRUE)) From 867e860996d8abb6477c06dc0a29436f8bf54242 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 9 Oct 2023 11:04:11 -0400 Subject: [PATCH 2/2] NEWS bullet --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 6a344f21b..4727ce794 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # vctrs (development version) +* Fixed an issue with complex vector tests related to changes in R-devel + (#1883). + * Added a class to the `vec_locate_matches()` error that is thrown when an overflow would otherwise occur (#1845).