search

tidymodels / broom

Convert statistical analysis objects from R into tidy format
https://broom.tidymodels.org
Other
1.46k stars 304 forks source link

Add support for `conf.level` in `augment.lm` #1191

Closed zietzm closed 7 months ago

zietzm commented 9 months ago

Addresses https://github.com/tidymodels/broom/issues/949.

https://github.com/alexpghayes/modeltests/pull/38 added support for the conf.level argument in augment methods.

This PR adds support for user-specified conf.level in augment.lm, which previously returned only values for conf.level=0.95, without any user alert.

alexpghayes commented 8 months ago

Yeah, so my general philosophy is that all the utilities like augment_newdata() are enormous maintenance liabilities and the best move is to get rid of them. This seems like an excellent time to factor out the dependence of augment.lm() on augment_newdata(). I would just copy-paste the augment_newdata() implementation into augment.lm() and then apply these changes within the augment.lm() method only.

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 augment_newdata().

zietzm commented 8 months ago

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.

simonpcouch commented 8 months ago

Pasting the augment_newdata() definition into augment.lm() doesn't feel much like a win for maintainability to me. There are a few places in the augment_newdata() definition that are accommodating edge cases for other model objects and the process of trimming those out for only what's needed for lm sounds labor-intensive and error-prone.

Thanks for the quick updates, @zietzm. If you're game for just passing augment.lm()'s dots to augment_newdata() and documenting that those dots are now passed to predict.lm() I'll be game to review! Will go ahead and enable Actions.

zietzm commented 8 months ago

@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.

simonpcouch commented 8 months ago

Feel free to ignore the actions dependency errors; unrelated and looking into them right now.

zietzm commented 8 months ago

@simonpcouch, thanks for your help! Are any further changes needed here now that all tests are passing?

simonpcouch commented 8 months ago

Thanks again for the attention here, @zietzm! Every draft of yours looked solid. For consistency with the analogous argument in tidy() methods, I went ahead and made use of your work in alexpghayes/modeltests#38 and transitioned that argument name from predict()'s level to broom's usual conf.level. We can depend on a development version of modeltests until a new version makes it to CRAN. :)

github-actions[bot] commented 6 months ago

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.