-
Notifications
You must be signed in to change notification settings - Fork 88
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
let fit_xy()
take dgCMatrix input
#1121
Conversation
fit_xy()
take dgCMatrix input
test_that("to_sparse_data_frame() is used correctly", { | ||
skip_if_not_installed("LiblineaR") | ||
|
||
local_mocked_bindings( |
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.
The main testing strategy follows this template:
- mock the functions that deals with sparsevctrs
- see if we can trigger all paths inside those functions
set_engine("LiblineaR") | ||
|
||
expect_no_error( | ||
lm_fit <- fit_xy(spec, x = hotel_data[, -1], y = hotel_data[, 1]) |
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.
if this didn't work, it would take quite a lot longer to run which we would notice
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.
Would we? Does "didn't work" mean a failure or just inefficient?
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.
inefficient. it should pop up as a "this test is running a little long" from CRAN / CMD R Check
@@ -32,6 +32,7 @@ Imports: | |||
prettyunits, | |||
purrr (>= 1.0.0), | |||
rlang (>= 1.1.0), | |||
sparsevctrs (>= 0.1.0.9000), |
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.
it uses dev version because of this bug fix: r-lib/sparsevctrs@9c22ca9
sparsevctrs will of course be merged in time for parsnip release
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.
Can you add something to fit.model_spec()
to mention that this can happen? Also, does the description for x
need to reflect this?
Also, please add a note to the NEWS file.
set_engine("LiblineaR") | ||
|
||
expect_no_error( | ||
lm_fit <- fit_xy(spec, x = hotel_data[, -1], y = hotel_data[, 1]) |
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.
Would we? Does "didn't work" mean a failure or just inefficient?
added a little documentation. It will fleshed out more once the other parts of #1125 is added |
R/sparsevctrs.R
Outdated
if (allow_sparse(object)) { | ||
x <- sparsevctrs::coerce_to_sparse_data_frame(x) | ||
} else { | ||
cli::cli_warn(c( |
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 going to update the PR to make this a failure.
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. |
Ref: #1125
General idea:
TODO: