Adding gee tidiers
#958
Adding gee tidiers
#958
Conversation
|
Hi, @simonpcouch I'm struggling to make sure this will merge. I haven't installed all the dependent libraries for the tests (each individual modeling package), which I figured shouldn't cause an error during a CRAN check (but may error locally). However, there is a package dependency called My commit to the > checking package dependencies ... ERROR
Package suggested but not available: 'lfe'
The suggested packages are required for a complete check.
Checking can be attempted without them by setting the environment
variable _R_CHECK_FORCE_SUGGESTS_ to a false value.
See section 'The DESCRIPTION file' in the 'Writing R Extensions'
manual.
1 error x | 0 warnings v | 0 notes v
Error: Error: R CMD check found ERRORs
Execution halted
Error: Process completed with exit code 1. |
|
Thanks for the PR! Well done. You're fine to ignore the I'm seeing 22 files changed at the moment, mostly for documentation updates for packages other than I'm not able to review the PR more fully at the moment, but will get to this as soon as I can! |
|
Also, could you give a go at unit tests for these tidiers using the |
|
Related to bbolker/broom.mixed#110—I'm wondering if these are better suited to broom.mixed. |
shah-in-boots
force-pushed
the
gee-tidiers
branch
from
6008d89 to
a82c06d
Compare
|
Yeah, I'll add the unit tests as well - thanks for reminding me. About #bbolker/broom.mixed#110, it should likely go wherever |
|
I believe this is working now - added tests as well. The area of weakness here I believe is |
| #' @template title_desc_tidy | ||
| #' | ||
| #' @param x A `gee` object returned from a call to [gee::gee()]. | ||
| #' @param conf.int Logical indicating whether or not to include a confidence |
There was a problem hiding this comment.
Would you use templates for parameter documentation for the standard arguments? See https://github.com/tidymodels/broom/blob/master/R/biglm-tidiers.R#L6 for an example
| tibble::as_tibble(rownames = "term") %>% | ||
| dplyr::select(-c(`Naive S.E.`, `Naive z`)) | ||
|
|
||
| # Remove naive estimators |
There was a problem hiding this comment.
Can you expand this comment to include why we should remove the naive estimators?
| coefs$conf.high <- coefs$estimate + zstat * coefs$std.error | ||
| } | ||
|
|
||
| if (exponentiate) { |
There was a problem hiding this comment.
We have a new approach to coefficient exponentiation which it would be nice to use here. See https://github.com/tidymodels/broom/blob/master/R/biglm-tidiers.R#L57 for an example.
| @@ -0,0 +1,28 @@ | |||
| context("gee-gee") | |||
There was a problem hiding this comment.
As of testthat version 3 calls to context() are no longer necessary :)
|
|
||
| g1 <- gee::gee(mpg ~ wt + hp, id = cyl, data = mtcars, corstr = "exchangeable") | ||
|
|
||
| # Tidy |
There was a problem hiding this comment.
I think this comment (and similarly for glance) is probably unnecessary as it is already very clear what your tests do :)
|
Closing in favor of bbolker/broom.mixed#110. :) |
|
This pull request has been automatically locked. If you believe the issue addressed here persists, please file a new PR (with a reprex: https://reprex.tidyverse.org) and link to this one. |
Added tidy, glance, and basic augment function for gee. However, as
predict()does not currently work forgee::gee(), nor does the model object return the predictor values, augment is limited. In addition,glance()can be improved upon if there are some agreement with how to assess quality of these clusters (such as a Hosmer-Lemeshow test).Please let me know if anything else is needed.