-
Notifications
You must be signed in to change notification settings - Fork 303
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
Support for car::lht and better term handling #1106
Conversation
@grantmcdermott, thank you for this PR! I apologize for not getting back to your original issue—as always, very well-considered and -argued. I'm on board for this as well as the I appreciate you linking out to the modelsummary issue; good to know where the both of yall are at in terms of how you think about broom's lifecycle and reliability. I pushed one small change to pass the "hard" check. [edit: I had said I would request a change, but I see why your logic is the way it is now.🙂] |
Feel free to ignore the pkgdown checks—that machinery needs updating.🌝🌚 |
Thanks Si! I can add the
Lmk your thoughts and I'll try to submit ASAP. |
On board!
On board!
I think I'd hold off on this. These columns have been around, at least by position, since some of the first commits to broom. I see the argument for why they shouldn't be there, but I'd imagine this would affect a good few reverse dependencies. |
just to let you know i haven't forgotten this... need to get grading done first, though :'-| |
@grantmcdermott No rush. :) |
Thanks for bearing with me @simonpcouch. I think these last few changes should do it. I added the following note to the #' Note that the output of `glance.anova()` will vary depending on the initializing
#' anova call. In some cases, it will just return an empty data frame. In other
#' cases, `glance.anova()` may return columns that are also common to
#' `tidy.anova()`. This is partly to preserve backwards compatibility with early
#' versions of `broom`, but also because the underlying anova model yields
#' components that could reasonably be interpreted as goodness-of-fit summaries
#' too. Example: devtools::load_all("~/Documents/Projects/broom")
#> ℹ Loading broom
a <- lm(mpg ~ wt + qsec + disp, mtcars)
b <- lm(mpg ~ wt + qsec, mtcars)
ab <- anova(a, b)
glance(ab)
#> # A tibble: 1 × 2
#> deviance df.residual
#> <dbl> <dbl>
#> 1 195. 29
## Example where glance returns an empty DF
glance(anova(a))
#> # A tibble: 0 × 0 Created on 2022-06-13 by the reprex package (v2.0.1) |
Awesome—I'm away from work for the week but will give this a more thorough look + merge if things look good next week. :) |
This looks great! No edits from me—will just update Was a little bit nervous about the
Woop woop! This may be breaking for some non-testable dependencies, but this feels like a change worth making. |
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. |
(Mostly) fixes #1090.
I was very tempted to create a new, dedicated
glance.anova
method and pull some columns that are currently returned as part of thetidy.anova
object. (Most obviously, the "df.residual" and "rss" columns.) But for now I'll stick to a more conservative fix that just supportscar::linearHypothesis
and returns more information about the model contrasts. OTOH, the fact thatglance.anova
currently still returns a not very sensible data frame is probably something that needs to be fixed at some point.Some examples:
Created on 2022-06-06 by the reprex package (v2.0.1)