easystats / insight

:crystal_ball: Easy access to model information for various model objects
https://easystats.github.io/insight/
GNU General Public License v3.0
380 stars 38 forks source link

Fix compact_list() for labelled + other vctrs classes #880

Open bwiernik opened 1 month ago

bwiernik commented 1 month ago

Closes https://github.com/easystats/performance/issues/727

strengejacke commented 1 month ago

Thanks, however, there are a few failing tests related to this PR:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-compact-list.R:4:3'): compact_list works as expected ─────────
compact_list(list(NULL, 1, list(NULL, NULL))) (`actual`) not equal to list(1) (`expected`).

`actual` is length 2
`expected` is length 1

`actual[[2]]` is a list
`expected[[2]]` is absent
── Failure ('test-is-empty-object.R:10:3'): is_empty_object works as expected ──
is_empty_object(list(NULL, list(NULL, NULL))) is not TRUE

`actual`:   FALSE
`expected`: TRUE 

[ FAIL 2 | WARN 0 | SKIP 26 | PASS 3335 ]
Error: Error: Test failures
Execution halted
bwiernik commented 1 month ago

Yeah saw that -- was doing this on a plane and didn't have a chance to investigate the tests before landing

strengejacke commented 1 month ago

That's why I don't like tidyverse... breaks standard R code in so many instances, and people are not aware it's a tidyverse problem.

bwiernik commented 1 month ago

It's mostly just vctrs that does that because of the strong typing

strengejacke commented 1 month ago

I think the easiest thing is just to remove the vctrs-attributes before running the function: https://github.com/easystats/insight/pull/879/commits/45499b100ee6438a31cb15dbe20eb358bcca83a3

bwiernik commented 1 month ago

@strengejacke I'm worried that that will have unforseen consequences. We should adjust the i == "NULL" check so that the attributes aren't removed from the returned object.

Why exactly are we comparing against the character value "NULL"? Is it just so that lists of all NULLs return true? (ala any(list(NULL, NULL) == "NULL")). If so, lets' do this instead:

any(sapply(i, is.null), na.rm = TRUE)

If we do need to retain the comparison against character "NULL" for another reason, let's do one of these:

as.character(i) == "NULL"

or: i %==% "NULL", defined as:

`%==%` <- function(e1, e2) {
  e1[] <- sapply(e1, \(x) {class(x) <- setdiff(class(x), c("haven_labelled", "vctrs_vctr")); return(x)})
  e2[] <- sapply(e2, \(x) {class(x) <- setdiff(class(x), c("haven_labelled", "vctrs_vctr")); return(x)})
  `==`(e1, e2)
}
strengejacke commented 1 month ago

I'm not sure which exact situation is caught, I remember we had to do this. Probably when deparse(NULL) is part of the list?

strengejacke commented 1 month ago

Seems like we had similar tries, but reverted: https://github.com/easystats/insight/commit/fc723c88e130359fb7fabc08fd38d92fd51049c2

strengejacke commented 4 weeks ago

Difficult to track, it was even discussed when the functions were in datawizard: https://github.com/easystats/datawizard/pull/52#issuecomment-1020312115

bwiernik commented 4 weeks ago

Okay, in that case, let's keep the check against character "NULL" and head off the the vctrs type-checking by using is.character(i) && i == "NULL" (which is probably what vctrs would want us to do in this situation anyways).

bwiernik commented 4 weeks ago

(I am not in principle opposed to vctrs' strictness around type comparisons; it is better practice than relying on implicit coercion. I agree with tidyverse team that R's ubiquitous type coercion causes more problems than it solves. But I wish that the error messages were more helpful at diagnosing that is the issue.)

strengejacke commented 4 weeks ago

When you look at the commit history of that file, you'll see that we had similar attempts in the past: https://github.com/easystats/insight/commit/8de4c03d5146c01a7b4622fa5ebe0b48224a2413#diff-39fae9f4745e94f659659b4534624f2d8158af635060c681ec48ec56b9bdf4d8R10

Not sure why we ended up with the current solution

strengejacke commented 4 weeks ago

Neither your approach nor mine work correctly. See this result, either for this PR, or #881:

out <- lapply(
  mtcars[, 1:3, drop = FALSE],
  bayestestR::ci,
  ci = c(0.9, 0.8),
  verbose = FALSE
)
insight::compact_list(out)
#> named list()

Expected result

out <- lapply(
  mtcars[, 1:3, drop = FALSE],
  bayestestR::ci,
  ci = c(0.9, 0.8),
  verbose = FALSE
)
insight::compact_list(out)
#> $mpg
#> Equal-Tailed Interval
#> 
#> 90% ETI        |        80% ETI
#> -------------------------------
#> [12.00, 31.30] | [14.34, 30.09]
#> 
#> $cyl
#> Equal-Tailed Interval
#> 
#> 90% ETI      |      80% ETI
#> ---------------------------
#> [4.00, 8.00] | [4.00, 8.00]
#> 
#> $disp
#> Equal-Tailed Interval
#> 
#> 90% ETI         |         80% ETI
#> ---------------------------------
#> [77.35, 449.00] | [80.61, 396.00]