remove finish_glance and add nobs

[vincentarelbundock]

Following up on #594, I wanted to see how much work it would be to replace finish_glance with individual calls to stats::BIC et al. inside every glance method (with @evalRd return_glance). Turned out not to be that bad (well, it was pretty bad, but I'm quite far along now). I also improved the docs in some places along the way.

Testing locally is difficult for some reason, so I'm opening this WIP PR just to see how far I am to a clean change.

[vincentarelbundock]

This PR does the following:

1. Remove finish_glance completely
2. Move the AIC, BIC, logLik, deviance, df.residual calls to model-specific glance methods.
3. Add nobs using stats::nobs to the vast majority of glance methods (all the ones I could figure out). Wherever possible, nobs is added last to preserve the column order of the other glance entries.
4. Add "nobs" to return_glance
5. Add a R/nobs.R file which includes nobs methods for models that do not export one.
6. Minor updates to the tests. Mostly, this involved changing the hard-coded number of columns in glance output.

Travis gives us the OK.

There are two doc-related warnings which might have been introduced by this PR. I can't quite figure out how to make them disappear. Any tips would be much appreciated.

* checking Rd files ... WARNING
prepare_Rd: lme4_tidiers.Rd: Only one \value section is allowed: the first will be used
prepare_Rd: ordinal_tidiers.Rd: Only one \value section is allowed: the first will be used

@alexpghayes Please let me know if there's anything you'd like me to improve.

[gavinsimpson]

Re the multiple \value sections, isn't this because you document all the functions in the same rd file lme4-tidiers.R and there are multiple @return tag and @evalRd, which results in multiple \value sections in the rendered .Rd file, e.g. Line:42 and Line:113

I think the first one is coming from the @return in

broom/R/lme4-tidiers.R

Line 55 in ad94acd

#' @return `tidy` returns one row for each estimated effect, either

and the second from the @evalRd in

broom/R/lme4-tidiers.R

Line 283 in ad94acd

#' @evalRd return_glance(

) in R/lme4-tidiers.R

[vincentarelbundock]

Thanks for looking at this.

I’m not super familiar with roxygen2, so I guess my question is why the Rd file gets written with 2 value sections.

The return section you point to here is attached to the tidy method.

broom/R/lme4-tidiers.R

Line 55 in ad94acd

#' @return `tidy` returns one row for each estimated effect, either

The return section (with evalRd) you point to here is attached to the glance method:

broom/R/lme4-tidiers.R

Line 283 in ad94acd

#' @evalRd return_glance(

This seems like a similar setup to the one used in ordinal-tidiers.R:

broom/R/ordinal-tidiers.R

Line 47 in ad94acd

#' @return

broom/R/ordinal-tidiers.R

Line 126 in ad94acd

#' @evalRd return_glance(

But I don’t get a warning in that case.

More generally, I’m curious to know what is considered “proper” broom documenting style. For instance, the plm-tidiers.R roxygen looks a lot different than either ordinal or lme4. IMHO, plm seems clearer, but I’m more looking to conform…

[alexpghayes]

Will give this a thorough look in a day or two!

[alexpghayes]

Okay, so I'm definitely on grad student time and I apologize for that!

This is fantastic! Will you:

• Add yourself as a contributor in DESCRIPTION
• Add a note in NEWS that many glance methods now return the number of observations in an nobs column, which is typically the rightmost column

After that I'll merge!

[vincentarelbundock]

cool cool. I did those two things and fixed some minor conflicts against master. Also removed lme4 and nlme4 which appear to have been deprecated.

Travis breaks, but two of their machines break and lavaan, which I have not touched, and one says robust package missing.

Here's what the tests look like on my machine: https://gist.github.com/vincentarelbundock/dee3cb1dbb3634c442f595f3ddc39310

[alexpghayes]

Thank you so much! Will clean up breakages myself!

[topepo]

Were reverse dependencies checked for this change? There are some packages that will break such as sweep. @mdancho84

[alexpghayes]

That was expected. This is a very good change.

[simonpcouch]

There are a couple reverse dependencies broken, yes (#862). Filing issues/PRs right now!

[github-actions]

This pull request 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.