vincentarelbundock / modelsummary

Beautiful and customizable model summaries in R.
http://modelsummary.com
Other
912 stars 77 forks source link

Output of mixed models for forthcoming *parameters* update #255

Closed strengejacke closed 3 years ago

strengejacke commented 3 years ago

Hi, I'm planning to submit an update of parameters to CRAN very soon. Some things have changed (see https://github.com/easystats/parameters/blob/main/NEWS.md), of which some points might be breaking changes. In particular the behaviour for mixed models changed in model_parameters() (there's now an effects argument, to include random parameters).

Can you please check if your package is affected?

library(modelsummary)
library(lme4)
#> Loading required package: Matrix
m <- lmer(Reaction ~ Days + (Days | Subject), sleepstudy)

# new default
modelsummary(m, output = "data.frame")
#> Registered S3 method overwritten by 'broom.mixed':
#>   method      from 
#>   tidy.gamlss broom
#>         part                  term         statistic  Model 1
#> 1  estimates           (Intercept) modelsummary_tmp1  251.405
#> 2  estimates           (Intercept) modelsummary_tmp2  (6.825)
#> 3  estimates                  Days modelsummary_tmp1   10.467
#> 4  estimates                  Days modelsummary_tmp2  (1.546)
#> 5  estimates       sd__(Intercept) modelsummary_tmp1   24.741
#> 6  estimates       sd__(Intercept) modelsummary_tmp2       ()
#> 7  estimates cor__(Intercept).Days modelsummary_tmp1    0.066
#> 8  estimates cor__(Intercept).Days modelsummary_tmp2       ()
#> 9  estimates              sd__Days modelsummary_tmp1    5.922
#> 10 estimates              sd__Days modelsummary_tmp2       ()
#> 11 estimates       sd__Observation modelsummary_tmp1   25.592
#> 12 estimates       sd__Observation modelsummary_tmp2       ()
#> 13       gof                   AIC                     1755.6
#> 14       gof                   BIC                     1774.8
#> 15       gof              Log.Lik.                   -871.814
#> 16       gof              REMLcrit                   1743.628

# how it probably should look like
modelsummary(m, effects = "fixed", output = "data.frame")
#>        part        term         statistic  Model 1
#> 1 estimates (Intercept) modelsummary_tmp1  251.405
#> 2 estimates (Intercept) modelsummary_tmp2  (6.825)
#> 3 estimates        Days modelsummary_tmp1   10.467
#> 4 estimates        Days modelsummary_tmp2  (1.546)
#> 5       gof         AIC                     1755.6
#> 6       gof         BIC                     1774.8
#> 7       gof    Log.Lik.                   -871.814
#> 8       gof    REMLcrit                   1743.628

Created on 2021-04-07 by the reprex package (v2.0.0)

vincentarelbundock commented 3 years ago

Thanks for the note @strengejacke, I really appreciate it!

This is a super neat feature, but shouldn't the default be "all"? AFAICT, the basic summary method does print random components by default, so as long as their not too expensive to compute that would seem natural.

Also, if I call parameters(x, effects = "all") on a model that doesn't support the argument, will effects be ignored and sunk in the ellipsis black hole? (Desirable, I think.)

strengejacke commented 3 years ago

Also, if I call parameters(x, effects = "all") on a model that doesn't support the argument, will effects be ignored and sunk in the ellipsis black hole? (Desirable, I think.)

Yes. That's why the behaviour for model_summary() now seems to have changed, because model_parameters() for mixed models did not have an effects argument before.

This is a super neat feature, but shouldn't the default be "all"?

That might be the better default, and it's not time consuming to compute. @IndrajeetPatil @DominiqueMakowski what do you say?

IndrajeetPatil commented 3 years ago

Yes! I would also vote for parameters(x, effects = "all") being the default.

vincentarelbundock commented 3 years ago

Commit https://github.com/vincentarelbundock/modelsummary/commit/ef9a398ed6faf31aabc17f96d99a16f13476b824 ensures compatibility with the future version of parameters.

I also see that a new commit has made "all" the default (great!!), so I'm closing this issue.

Thanks again for the note!

strengejacke commented 3 years ago

Yes, there were a lot of commits the past days, mainly fixing print issues (and adding related tests) and fixing minor issues. Now all local and GitHub checks pass, I just need to fix a revdep issue.

strengejacke commented 3 years ago

I cannot resolve the revdep issues. Changing the default to effects = "all" at the moment would break effectsize (which had two updates in the past three weeks, so I would rather wait with this breaking change until the next release cycle).

Thus, I would suggest you set effects explicitly, as the default will probably be effects = "fixed" for this update of parameters.

vincentarelbundock commented 3 years ago

No worries. I made the change you recommend and wrote a test to sniff out any problems if/when you move to "all" as default on your end.

vincentarelbundock commented 3 years ago

thanks again