-
Notifications
You must be signed in to change notification settings - Fork 304
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
Add support for conf.level
in augment.lm
#1191
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm feeling hesitant to make changes to augment_newdata()
, which underlies augmenting for many other methods in the package. I do wonder if we may be better off passing augment.lm
's dots to augment_newdata()
's, as augment.lm()
's dots are currently unused and not checked for emptiness--this would allow for passing conf.level
without requiring any changes to augment_newdata
.
Open to your opinions, @alexpghayes.
Yeah, so my general philosophy is that all the utilities like I know this approach sounds like it creates a ton of copy-pasted code, but I swear it is the right move for broom. Every model object behaves a little bit differently, and the best way to handle this is for each model object to have genuinely custom/standalone tidier implementations that don't rely on things like |
Updated with @alexpghayes copy-paste suggestion. I tried to make only minimal changes to the pasted code. Happy to clean it up more if desired. |
Pasting the Thanks for the quick updates, @zietzm. If you're game for just passing |
@simonpcouch, either approach works for me! This is certainly the simpler approach. Updated the PR and happy to make any more changes as you see fit. |
Feel free to ignore the actions dependency errors; unrelated and looking into them right now. |
@simonpcouch, thanks for your help! Are any further changes needed here now that all tests are passing? |
Thanks again for the attention here, @zietzm! Every draft of yours looked solid. For consistency with the analogous argument in |
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. |
Addresses #949.
alexpghayes/modeltests#38 added support for the
conf.level
argument inaugment
methods.This PR adds support for user-specified
conf.level
inaugment.lm
, which previously returned only values forconf.level=0.95
, without any user alert.