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

Anova mermod fix #1115

Merged
merged 5 commits into from
Aug 29, 2022
Merged

Anova mermod fix #1115

merged 5 commits into from
Aug 29, 2022

Conversation

bbolker
Copy link
Contributor

@bbolker bbolker commented Aug 23, 2022

This addresses #1114

@bbolker
Copy link
Contributor Author

bbolker commented Aug 23, 2022

Ugh, needs more work. I made some unwarranted assumptions.

@simonpcouch
Copy link
Collaborator

Thanks for getting this started!

Ugh, needs more work. I made some unwarranted assumptions.

I felt this.🫠 Working with this output can be really tricky—I wish we had some more attributes or classes to work with.

Let me know if you'd appreciate me spending some time here! Glad to review whenever feels good to you.

@bbolker
Copy link
Contributor Author

bbolker commented Aug 23, 2022

I could solve this problem on the lme4 end by making anova.merMod() return an object with class equal to c("anova.mer", "anova", "data.frame"), and writing a separate method for it to be included in broom.mixed ... would that be better? (The only downside is that it would presumably copy a lot of code from the tidy.anova() method ...)

Copy link
Collaborator

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good approach! re: the copying code downside, much of the tidy.anova source is now the Special catch for car::linearHypothesis conditional (which I presume you wouldn't need?) and I could export the renamers object as anova_tidier_renamers so that that vector would automatically be synced up. Another downside, though, is that this would require users to load broom.mixed in a place where they used to be able to get by with only the broom package loaded, and the reason for that isn't visible in the sense that output from the anova() generic could require either package depending on the method dispatched to.

Another option, which is less expressive but is more backward-compatible and likely more maintainable, suggested in review:

R/stats-anova-tidiers.R Outdated Show resolved Hide resolved
@simonpcouch simonpcouch merged commit 005cdf3 into tidymodels:main Aug 29, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2022
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.

2 participants