Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

bwiernik
Copy link
Contributor

@bwiernik bwiernik commented Jun 1, 2024

@strengejacke
Copy link
Member

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
Copy link
Contributor Author

bwiernik commented Jun 2, 2024

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

@strengejacke
Copy link
Member

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
Copy link
Contributor Author

bwiernik commented Jun 3, 2024

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

@strengejacke
Copy link
Member

I think the easiest thing is just to remove the vctrs-attributes before running the function:
45499b1

@bwiernik
Copy link
Contributor Author

bwiernik commented Jun 3, 2024

@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
Copy link
Member

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
Copy link
Member

Seems like we had similar tries, but reverted:
fc723c8

@strengejacke
Copy link
Member

Difficult to track, it was even discussed when the functions were in datawizard: easystats/datawizard#52 (comment)

Use `is.character(i) || …` to head off vctrs-induced type mismatch
@bwiernik
Copy link
Contributor Author

bwiernik commented Jun 3, 2024

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
Copy link
Contributor Author

bwiernik commented Jun 3, 2024

(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
Copy link
Member

When you look at the commit history of that file, you'll see that we had similar attempts in the past:
8de4c03#diff-39fae9f4745e94f659659b4534624f2d8158af635060c681ec48ec56b9bdf4d8R10

Not sure why we ended up with the current solution

@@ -10,9 +10,9 @@
#' @export
compact_list <- function(x, remove_na = FALSE) {
if (remove_na) {
x[!sapply(x, function(i) !is_model(i) && !inherits(i, c("Formula", "gFormula")) && (length(i) == 0L || is.null(i) || (length(i) == 1L && is.na(i)) || all(is.na(i)) || any(is.character(i) && i == "NULL", na.rm = TRUE)))]
x[!sapply(x, function(i) !is_model(i) && !inherits(i, c("Formula", "gFormula")) && (length(i) == 0L || is.null(i) || (length(i) == 1L && is.na(i)) || all(is.na(i)) || all(sapply(i, is.null)) || any(sapply(i, \(j) length(j) == 1 && is.character(j) && j == "NULL"), na.rm = TRUE)))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use lambdas here due to backwards compatibility.

@strengejacke
Copy link
Member

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]

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check_model fails if dependent variable is labelled
2 participants