-
Notifications
You must be signed in to change notification settings - Fork 23
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 toggle_sparsity()
#281
Conversation
R/sparsevctrs.R
Outdated
if (is.null(model) || model == "ranger") { | ||
return("no") | ||
} |
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.
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.
Loving where this is going! Finding the inlined model fit very satisfying😆
I know that we're planning on testing all of the unique combinations of these code paths in extratests, but could we test toggle_sparsity()
by itself here as well as one example workflow fit where toggle_sparsity()
returns "yes"
?
Co-authored-by: Simon P. Couch <[email protected]>
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.
Awesome! Just about there, though still would like to the testing comment in my review description resolved.
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.
Looking great! Thumbs up from me on merging once you've resolved those test failures.
tests/testthat/test-sparsevctrs.R
Outdated
|
||
data("ames", package = "modeldata") | ||
|
||
tree_spec <- parsnip::boost_tree("regression", "xgboost") |
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.
Seeing:
══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-sparsevctrs.R:294:3'): toggle_sparsity doesn't break fit ─────
Expected `fit(wf_spec, ames)` to run without any errors.
i Actually got a <rlang_error> with text:
Please install the xgboost package to use this engine.
in tests.
A few of these read skip_if_not_installed("glmnet")
but then use an xgboost model spec.
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
to close #271
I think this is the last puzzle piece to make sparsity useful in tidymodels.
This will toggle the sparsity creation of recipes based on whether or not we estimate it will be useful or not.