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

remove finish_glance and add nobs #597

Merged
merged 5 commits into from Mar 6, 2019
Merged

remove finish_glance and add nobs #597

merged 5 commits into from Mar 6, 2019

Conversation

vincentarelbundock
Copy link
Contributor

@vincentarelbundock vincentarelbundock commented Jan 28, 2019

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
Copy link
Contributor Author

@vincentarelbundock vincentarelbundock commented Jan 28, 2019

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.

@vincentarelbundock vincentarelbundock changed the title WIP: remove finish_glance and add nobs remove finish_glance and add nobs Jan 29, 2019
@gavinsimpson
Copy link
Contributor

@gavinsimpson gavinsimpson commented Jan 30, 2019

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

#' @return `tidy` returns one row for each estimated effect, either
and the second from the @evalRd in
#' @evalRd return_glance(
) in R/lme4-tidiers.R

@vincentarelbundock
Copy link
Contributor Author

@vincentarelbundock vincentarelbundock commented Jan 30, 2019

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.

#' @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:

#' @evalRd return_glance(

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

#' @return

#' @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
Copy link
Collaborator

@alexpghayes alexpghayes commented Jan 30, 2019

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

@alexpghayes
Copy link
Collaborator

@alexpghayes alexpghayes commented Mar 5, 2019

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
Copy link
Contributor Author

@vincentarelbundock vincentarelbundock commented Mar 5, 2019

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 alexpghayes merged commit 67cbbd3 into tidymodels:master Mar 6, 2019
0 of 2 checks passed
@alexpghayes
Copy link
Collaborator

@alexpghayes alexpghayes commented Mar 6, 2019

Thank you so much! Will clean up breakages myself!

@topepo
Copy link
Member

@topepo topepo commented May 19, 2020

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

@alexpghayes
Copy link
Collaborator

@alexpghayes alexpghayes commented May 19, 2020

That was expected. This is a very good change.

@simonpcouch
Copy link
Collaborator

@simonpcouch simonpcouch commented May 27, 2020

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

@github-actions
Copy link

@github-actions github-actions bot commented Mar 7, 2021

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.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 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

Successfully merging this pull request may close these issues.

None yet

5 participants