Closed hadley closed 1 year ago
Hi @hadley, thanks for raising this and for the tip about the testthat helpers. I've relocated nowarn_on_ci()
as you suggested, hopefully that gets things running for you?
The issue seems to be that the output from map_lgl()
no longer has names in the dev version, which then results in an unexpectedly empty error message. The call in question is here:
https://github.com/dmphillippo/multinma/blob/f3f43d455470c695cedf47c24cc5b7a64680fe25/R/nma_data.R#L698-L706
I'm not quite sure why the result posdef
isn't named in this instance, I haven't managed to recreate the issue outside of the function. Perhaps it has something to do with agd_Sigma
being constructed using by(., simplify=FALSE)
, so it isn't quite a bare list of matrices?
In any case, I can fix from my end if necessary by changing names(posdef[!posdef]))
to names(agd_Sigma[!posdef]))
in the glue call.
Hmmm, what sort of object is agd_Sigma()
? Is there any chance it's an S4 object?
It looks like purrr:::vctrs_vec_compat()
is stripping the names
@dmphillippo that was my suspicion too, but when I re-read the source code, I can't see which branch would remove names.
agd_Sigma
is a list of matrices, produced using by()
with simplify = FALSE
, so I don't think it's S4?
I've seen some problems with by()
elsewhere, but it normally looks more like this:
x <- by(data.frame(x = 1:3), 1:3, \(x) 1, simplify = FALSE)
names(x)
#> [1] "1" "2" "3"
purrr::map_lgl(x, \(y) TRUE)
#> Error in `purrr::map_lgl()`:
#> ! `.x` must be a vector, not a <by> object.
#> Backtrace:
#> ▆
#> 1. └─purrr::map_lgl(x, function(y) TRUE)
#> 2. └─purrr:::map_("logical", .x, .f, ..., .progress = .progress)
#> 3. └─vctrs::vec_assert(.x, arg = ".x", call = ..error_call)
#> 4. └─vctrs:::stop_scalar_type(x, arg, call = call)
#> 5. └─vctrs:::stop_vctrs(...)
#> 6. └─rlang::abort(message, class = c(class, "vctrs_error"), ..., call = vctrs_error_call(call))
Created on 2022-11-03 with reprex v2.0.2
Could you give me a little reprex to produce something that looks like agd_Sigma
?
Sure, here's a quick reprex. Looking more closely, I had unclass()
-ed the output from by()
to get a bare list, which was working with previous versions of purrr (i.e. posdef
retained the names):
df <- data.frame(a = c(1, 2, 2))
(agd_Sigma <- unclass(by(df, df$a, \(x) diag(nrow(x)), simplify = FALSE)))
#> $`1`
#> [,1]
#> [1,] 1
#>
#> $`2`
#> [,1] [,2]
#> [1,] 1 0
#> [2,] 0 1
#>
#> attr(,"call")
#> by.data.frame(data = df, INDICES = df$a, FUN = function(x) diag(nrow(x)),
#> simplify = FALSE)
(posdef <- purrr::map_lgl(agd_Sigma, ~all(eigen(., only.values = TRUE)$values > sqrt(.Machine$double.eps))))
#> [1] TRUE TRUE
names(posdef)
#> NULL
Created on 2022-11-04 by the reprex package (v2.0.1)
If lists produced using by()
aren't supported by purrr (should they be?) then I can find another way to create this list of matrices (I think dplyr::group_map()
should work).
No, that's definitely a bug, and I'll fix it in purrr: https://github.com/tidyverse/purrr/issues/993
Oooh, it's because the result is both a list and a 1d array and the names are stored in the dimnames which are stripped when I set dim(x) <- NULL
.
Brilliant, thanks @hadley 🙂
I noticed that too when I dug a little deeper - never seen that before!
I see:
If you can come up with a simple reprex, I'm happy to help debug it, but I spent 15 minutes with your package and couldn't get a clean test run. At a minimum, you need to move
nowarn_on_ci()
out oftestthat.R
and intotests/testthat/helpers-ci.R
or similar.