Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

lavaan update breaks glance.lavaan #835

Closed
yrosseel opened this issue Apr 23, 2020 · 9 comments
Closed

lavaan update breaks glance.lavaan #835

yrosseel opened this issue Apr 23, 2020 · 9 comments

Comments

@yrosseel
Copy link

yrosseel commented Apr 23, 2020

The glance.lavaan() function (in R/lavaan-tidiers.R) is accessing internal slots of a fitted lavaan object to extract some information, instead of using the extractor functions. As the internal slots will change in the next update, this breaks the glance.lavaan() function.

The urgent issue is line 136

converged = x@Fit@converged

which should be replaced by

converged = lavInspect(x, "converged")

as the 'Fit' slot is going to be removed.

But I would suggest changing the whole tibble section to this:

tibble(
    converged = lavInspect(x, "converged"),
    estimator = lavInspect(x, "options")$estimator,
    ngroups = lavInspect(x, "ngroups"),
    missing_method = lavInspect(x, "options")$missing,
    nobs = sum(purrr::accumulate(lavInspect(x, "nobs"), sum)),
    norig = sum(purrr::accumulate(lavInspect(x, "norig"), sum)),
    nexcluded = norig - nobs
)

avoiding any access to internal slots.

Finally, I don't understand why the purrr::accumulate() function is needed. I believe that what is intended is simply:

nobs = sum(lavInspect(x, "nobs"),
norig = sum(lavInspect(x, "norig"),
nexcluded = norig - nobs

taking the sum over the (possibly multiple) groups.

Yves.

@alexpghayes
Copy link
Collaborator

alexpghayes commented Apr 23, 2020

Wonderful, thank you, will make these updates!

@alexpghayes
Copy link
Collaborator

alexpghayes commented Apr 23, 2020

@yrosseel Would you be willing to export tidy.lavaan() and glance.lavaan() from lavaan itself, rather than having them live in broom?

@yrosseel
Copy link
Author

yrosseel commented Apr 24, 2020

Hm. No. Because I want to avoid adding additional package dependencies (eg tibble, dplyr, ...).

@simonpcouch
Copy link
Collaborator

simonpcouch commented May 25, 2020

Hello! Working through this update right now. It looks like the results from purrr::accumulate() and your proposal are indeed different, though:

library(lavaan)
#> This is lavaan 0.6-6
#> lavaan is BETA software! Please report any bugs.
library(broom)
library(tidyverse)

cfa.fit <- cfa(
  'F =~ x1 + x2 + x3 + x4 + x5',
  data = HolzingerSwineford1939, group = "school"
)

# version with purrr
glance.lavaan <- function(x, ...) {
  x %>%
    lavaan::fitmeasures(
      fit.measures =
        c(
          "npar",
          "chisq",
          "rmsea",
          "rmsea.ci.upper",
          "srmr",
          "aic",
          "bic",
          "tli",
          "agfi",
          "cfi"
        )
    ) %>%
    tibble::enframe(name = "term") %>%
    pivot_wider(id_cols = term, names_from = term, values_from = value) %>%
    select(order(colnames(.))) %>%
    map_df(as.numeric) %>%
    bind_cols(
      tibble(
        converged = lavInspect(x, "converged"),
        estimator = lavInspect(x, "options")$estimator,
        ngroups = lavInspect(x, "ngroups"),
        missing_method = lavInspect(x, "options")$missing,
        nobs = sum(purrr::accumulate(lavInspect(x, "nobs"), sum)),
        norig = sum(purrr::accumulate(lavInspect(x, "norig"), sum)),
        nexcluded = norig - nobs
      )
    ) %>%
    rename(rmsea.conf.high = rmsea.ci.upper, AIC = aic, BIC = bic)
}

old <- glance(cfa.fit)

# version with only sum
glance.lavaan <- function(x, ...) {
  x %>%
    lavaan::fitmeasures(
      fit.measures =
        c(
          "npar",
          "chisq",
          "rmsea",
          "rmsea.ci.upper",
          "srmr",
          "aic",
          "bic",
          "tli",
          "agfi",
          "cfi"
        )
    ) %>%
    tibble::enframe(name = "term") %>%
    pivot_wider(id_cols = term, names_from = term, values_from = value) %>%
    select(order(colnames(.))) %>%
    map_df(as.numeric) %>%
    bind_cols(
      tibble(
        converged = lavInspect(x, "converged"),
        estimator = lavInspect(x, "options")$estimator,
        ngroups = lavInspect(x, "ngroups"),
        missing_method = lavInspect(x, "options")$missing,
        nobs = sum(lavInspect(x, "nobs")),
        norig = sum(lavInspect(x, "norig")),
        nexcluded = norig - nobs
      )
    ) %>%
    rename(rmsea.conf.high = rmsea.ci.upper, AIC = aic, BIC = bic)
}

new <- glance(cfa.fit)

old$nobs == new$nobs
#> [1] FALSE
old$norig == new$norig
#> [1] FALSE

Created on 2020-05-25 by the reprex package (v0.3.0.9001)

Will be pushing some changes and referencing this issue here soon.🙂

@yrosseel
Copy link
Author

yrosseel commented May 26, 2020

Thanks! The purrr::accumulate() version was always wrong. So the 'new' version is correct!

@simonpcouch
Copy link
Collaborator

simonpcouch commented May 26, 2020

@benwhalley, any thoughts on nobs and norig representing the cumulative sum rather than just the sum of the groups? Just tagging you as you seem to be the implementer in #5932820.🙂

@benwhalley
Copy link

benwhalley commented May 26, 2020

I don't think I did implement that, but it's been a while if I did. No strong views on it now!

@simonpcouch
Copy link
Collaborator

simonpcouch commented May 26, 2020

Gotcha! Making the switch now, thanks.

@github-actions
Copy link

github-actions bot commented Mar 8, 2021

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants