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

Make sure sparse matrices can be used with predict() #1167

Merged
merged 8 commits into from
Sep 5, 2024

Conversation

EmilHvitfeldt
Copy link
Member

@EmilHvitfeldt EmilHvitfeldt commented Aug 30, 2024

Ref: #1125

Predicting with a sparse matrix already works, as long as the engine supports it. but is now tested with this PR.

Waiting for #1165 to be merged so I can use the helper function defined there to give better error message when sparse data is passed to model that doesn't support it

@EmilHvitfeldt EmilHvitfeldt marked this pull request as ready for review September 4, 2024 21:47
@EmilHvitfeldt
Copy link
Member Author

ended up adding sparse tibble support in this PR as well since we were almost there 2347eeb (#1167)

Comment on lines +447 to +449
if (inherits(x, "model_fit")) {
x <- x$spec
}
Copy link
Member Author

Choose a reason for hiding this comment

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

allow_sparse() was assumed to only work on model_spec. This change makes it so it also works on model_fit objects

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the additional context!

Copy link
Contributor

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

Stellar. Nice work!

tests/testthat/_snaps/sparsevctrs.md Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
Comment on lines +447 to +449
if (inherits(x, "model_fit")) {
x <- x$spec
}
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the additional context!

@EmilHvitfeldt EmilHvitfeldt merged commit a9aadfb into main Sep 5, 2024
10 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the sparse-matrix-predict branch September 5, 2024 15:43
Copy link

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.

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