cjvanlissa / tidySEM

55 stars 7 forks source link

update BLRT method #66

Closed haozhou1988 closed 1 year ago

haozhou1988 commented 1 year ago
  1. define the BLRT method for the MxRAMModel class to cope with Error in UseMethod("BLRT", x) : no applicable method for 'BLRT' applied to an object of class "c('MxRAMModel', 'MxModel')"
    
    library(tidySEM)
    #> Loading required package: OpenMx
    # only 1 class 
    res <- mx_mixture(
    model = "x ~ m{C}*1
           x ~~ v{C}*x",
    classes = 1,
    data = df
    )
    #> Running mix1 with 2 parameters

BLRT(res, replications = 4)

> Error in UseMethod("BLRT", x): no applicable method for 'BLRT' applied to an object of class "c('MxRAMModel', 'MxModel')"

2. adding a warning message when the user forgets to specify the argument name.
```r
library(tidySEM)
#> Loading required package: OpenMx
df <- iris[, 1, drop = FALSE]
names(df) <- "x"
res <- mx_mixture(model = "x ~ m{C}*1
                           x ~~ v{C}*x", 
                  class = 1:2, 
                  data = df
                  )
BLRT(res, 4)
#> Warning in BLRT.mixture_list(res, 4): Please check BLRT function, it should
#> include specific argument name. e.g., BLRT(res, replications = 4), not BLRT(res,
#> 4)
#>   diffLL diffdf  p
#> 1     NA     NA NA
#> 2     NA     NA NA
codecov[bot] commented 1 year ago

Codecov Report

Merging #66 (2785fea) into master (35f2763) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 2785fea differs from pull request most recent head bd2152c. Consider uploading reports for the commit bd2152c to get more accurate results

@@          Coverage Diff           @@
##           master     #66   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines           2       2           
======================================
  Misses          2       2           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

cjvanlissa commented 1 year ago

Thank you for these suggestions @haozhou1988 , but as you can see from the comments, I have some concerns about the sensibility of these changes.. What do you think about these concerns?

cjvanlissa commented 1 year ago

Two general points of feedback: 1) For wrapper functions like the current version of BLRT, it is good practice to just pass on all arguments to the wrapped function via ... 2) The problem with the current version of BLRT is not the user interface - it's the fact that mxCompare implements the bootstrapping inefficiently, and thus floods RAM and crashes the system for any realistic number of iterations.

If you check the dev folder, you see that I wrote a quicker paralellized implementation. But in the long term, we're working on simulation based inference for latent class analyses that should be MUCH faster than bootstrapping, and BLRT will become irrelevant.

haozhou1988 commented 1 year ago

Two general points of feedback:

  1. For wrapper functions like the current version of BLRT, it is good practice to just pass on all arguments to the wrapped function via ...
  2. The problem with the current version of BLRT is not the user interface - it's the fact that mxCompare implements the bootstrapping inefficiently, and thus floods RAM and crashes the system for any realistic number of iterations.

If you check the dev folder, you see that I wrote a quicker paralellized implementation. But in the long term, we're working on simulation based inference for latent class analyses that should be MUCH faster than bootstrapping, and BLRT will become irrelevant.

haozhou1988 commented 1 year ago

Looking forward to seeing the new blrt function. 👍