Skip to content

Conversation

EmilHvitfeldt
Copy link
Member

@EmilHvitfeldt EmilHvitfeldt commented Sep 5, 2024

Ref: #1125

This PR does two things.

First it adds an error if you try to use a sparse matrix with fit(), that redirects the user to use fit_xy().

Second, it passes the call argument through to_sparse_data_frame() for nicer errors.

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.

Leaving review as comment just to give us a chance to discuss the guidance to use fit_xy() before merging. Looks awesome. :)

@@ -1,5 +1,5 @@
to_sparse_data_frame <- function(x, object) {
if (methods::is(x, "sparseMatrix")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Digging this refactor. :)

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.

Kickass. Thanks for hearing me out on that fit() happy path. Sparsnip!!!

@EmilHvitfeldt EmilHvitfeldt merged commit 0b09e78 into main Sep 6, 2024
10 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the sparse-matrix-fit-error branch September 6, 2024 21:10
@github-actions
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 21, 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