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

mmrm() models #229

Merged
merged 10 commits into from
Nov 30, 2023
Merged

mmrm() models #229

merged 10 commits into from
Nov 30, 2023

Conversation

larmarange
Copy link
Owner

@larmarange larmarange commented Jul 27, 2023

fix #228

  • add unit tests

@ddsjoberg
Copy link
Collaborator

Hey hey! I am getting an error trying to run tidy_plus_plus().

# this version is from the mmrm branch ("refs/heads/mmrm")
packageVersion("broom.helpers")
#> [1] '1.13.1'

fit <- 
  mmrm::mmrm(
    formula = FEV1 ~ RACE + SEX + ARMCD * AVISIT + us(AVISIT | USUBJID),
    data = mmrm::fev_data
  )
summary(fit)
#> mmrm fit
#> 
#> Formula:     FEV1 ~ RACE + SEX + ARMCD * AVISIT + us(AVISIT | USUBJID)
#> Data:        mmrm::fev_data (used 537 observations from 197 
#> subjects with maximum 4 timepoints)
#> Covariance:  unstructured (10 variance parameters)
#> Method:      Satterthwaite
#> Vcov Method: Asymptotic
#> Inference:   REML
#> 
#> Model selection criteria:
#>      AIC      BIC   logLik deviance 
#>   3406.4   3439.3  -1693.2   3386.4 
#> 
#> Coefficients: 
#>                                Estimate Std. Error        df t value Pr(>|t|)
#> (Intercept)                    30.77748    0.88656 218.80000  34.715  < 2e-16
#> RACEBlack or African American   1.53050    0.62448 168.67000   2.451 0.015272
#> RACEWhite                       5.64357    0.66561 157.14000   8.479 1.56e-14
#> SEXFemale                       0.32606    0.53195 166.13000   0.613 0.540744
#> ARMCDTRT                        3.77423    1.07415 145.55000   3.514 0.000589
#> AVISITVIS2                      4.83959    0.80172 143.88000   6.037 1.27e-08
#> AVISITVIS3                     10.34211    0.82269 155.56000  12.571  < 2e-16
#> AVISITVIS4                     15.05390    1.31281 138.47000  11.467  < 2e-16
#> ARMCDTRT:AVISITVIS2            -0.04193    1.12932 138.56000  -0.037 0.970439
#> ARMCDTRT:AVISITVIS3            -0.69369    1.18765 158.17000  -0.584 0.559996
#> ARMCDTRT:AVISITVIS4             0.62423    1.85085 129.72000   0.337 0.736463
#>                                  
#> (Intercept)                   ***
#> RACEBlack or African American *  
#> RACEWhite                     ***
#> SEXFemale                        
#> ARMCDTRT                      ***
#> AVISITVIS2                    ***
#> AVISITVIS3                    ***
#> AVISITVIS4                    ***
#> ARMCDTRT:AVISITVIS2              
#> ARMCDTRT:AVISITVIS3              
#> ARMCDTRT:AVISITVIS4              
#> ---
#> Signif. codes:  0 '***' 0.001 '**' 0.01 '*' 0.05 '.' 0.1 ' ' 1
#> 
#> Covariance estimate:
#>         VIS1    VIS2    VIS3    VIS4
#> VIS1 40.5537 14.3960  4.9747 13.3867
#> VIS2 14.3960 26.5715  2.7855  7.4745
#> VIS3  4.9747  2.7855 14.8979  0.9082
#> VIS4 13.3867  7.4745  0.9082 95.5568

broom.helpers::tidy_plus_plus(fit)
#> ! `broom::tidy()` failed to tidy the model.
#> mmrm() registered as emmeans extension
#> Warning: The `full` argument of `model.frame.mmrm_tmb()` is deprecated as of mmrm 0.3.
#> ℹ The deprecated feature was likely used in the insight package.
#>   Please report the issue at <https://github.com/easystats/insight/issues>.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.
#> ✔ `tidy_parameters()` used instead.
#> ℹ Add `tidy_fun = broom.helpers::tidy_parameters` to quiet these messages.
#> Error in stats::model.matrix.default(model %>% model_get_terms(), data = stats::model.frame(model)): model frame and formula mismatch in model.matrix()

Created on 2023-08-04 with reprex v2.0.2

I'll try to look into it further this weekend 🍁

@larmarange
Copy link
Owner Author

Just for you information, #228 has not been merged into the main branch yet.

@larmarange
Copy link
Owner Author

Ok. I do not have an issue on my side, but I'm using mmrm version 0.2.2, the last version available on CRAN.

It seems that there is some change with the dev version.

Should we wait for the stabilisation and the release of version 0.3.0 before updating that PR?

Regards

@ddsjoberg
Copy link
Collaborator

Should we wait for the stabilisation and the release of version 0.3.0 before updating that PR?

Great suggestion. The author is back from leave this coming week. Perhaps we can update mmrm to have all the needed methods and correct the weights() dimension issue, then update.

@larmarange
Copy link
Owner Author

Perfect. Let's wait for the release of 0.3.0 then

@ddsjoberg
Copy link
Collaborator

hey hey @larmarange ! hope you're well!

mmrm made their release to CRAN, and i bumped the min version requirement to the CRAN version.

Would you like me to try and take it from here, or you prefer to do it?

@larmarange
Copy link
Owner Author

hey hey @larmarange ! hope you're well!

mmrm made their release to CRAN, and i bumped the min version requirement to the CRAN version.

Would you like me to try and take it from here, or you prefer to do it?

Hey

I'm currently traveling for work. Don't hesitate to look / test the PR if you have time.

I could myself look at it next week

Best

@larmarange
Copy link
Owner Author

We have to check but with the new version of the package, some dedicated methods in broom.helpers won't be needed anymore

@ddsjoberg ddsjoberg self-requested a review November 28, 2023 16:40
Copy link
Collaborator

@ddsjoberg ddsjoberg left a comment

Choose a reason for hiding this comment

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

Hi @larmarange !

I did some testing, and we were able to remove the custom methods (as you suggested), since I updated the methods in the mmrm package.

I added some unit tests (modeled after the betareg unit tests). Do you think we should add more?

@ddsjoberg
Copy link
Collaborator

@larmarange the tests that are failing are related to lme4, rather than mmrm FYI

@larmarange
Copy link
Owner Author

This is strange. I just deleted all caches and I'm trying to re run all jobs

@larmarange
Copy link
Owner Author

Everything seems OK for me.

Test failures are related to https://stackoverflow.com/questions/77481539/error-in-initializeptr-function-cholmod-factor-ldeta-not-provided-by-pack and not to the PR

So, for me, not a problem to merge this PR.

However, we will have to fix all checks before next release

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Merging #229 (2c82160) into main (99294ff) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #229   +/-   ##
=======================================
  Coverage   95.48%   95.48%           
=======================================
  Files          43       43           
  Lines        2680     2680           
=======================================
  Hits         2559     2559           
  Misses        121      121           
Files Coverage Δ
R/model_get_weights.R 93.75% <ø> (ø)

@ddsjoberg ddsjoberg merged commit 457c028 into main Nov 30, 2023
10 of 13 checks passed
@ddsjoberg ddsjoberg deleted the mmrm branch November 30, 2023 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for mmrm models
2 participants