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

MAINT Make ArgKminClassMode accept sparse datasets #27018

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Aug 5, 2023

Reference Issues/PRs

Follow-up of #24076.

What does this implement/fix? Explain your changes.

#24076 had a guard for the limitation on sparse datasets #23585 resolved, but #23585 was merged without #24076 being updated accordingly.

This PR removes this limiting guard.

Any other comments?

Even if ArgKminClassMode is the only class which overloads is_usable_for, I have not added tests to check this behavior not to complexify the test suite too much. Should I?

Also, do we need a changelog entry?

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@github-actions
Copy link

github-actions bot commented Aug 5, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: b47680c. Link to the linter CI: here

@jjerphan jjerphan marked this pull request as ready for review August 5, 2023 14:16
@jjerphan jjerphan added No Changelog Needed Quick Review For PRs that are quick to review labels Aug 5, 2023
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Also, do we need a changelog entry?

Yea, I think this needs one. This is an efficiency improvement for estimators that use ArgKMinClassMode with sparse matrices.

Copy link
Contributor

@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

LGTM pending changelog entry

@Micky774
Copy link
Contributor

Micky774 commented Aug 8, 2023

Even if ArgKminClassMode is the only class which overloads is_usable_for, I have not added tests to check this behavior not to complexify the test suite too much. Should I?

Actually we can avoid that by specifying ArgKminClassMode.valid_metrics() to avoid the not-yet-included Euclidean specializations.

Copy link
Contributor

@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

Needs changelog entry, and may we instead override the valid_metrics for the class to avoid needing to override is_usable_for in the first place.

jjerphan and others added 3 commits August 8, 2023 18:40
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Meekail Zain <Micky774@users.noreply.github.com>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan jjerphan force-pushed the maint/make-ArgKminClassMode-accept-sparse-datasets branch from 8eb58b0 to 72d1945 Compare August 13, 2023 18:48
Copy link
Contributor

@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 😄

@thomasjpfan thomasjpfan merged commit f5c4999 into scikit-learn:main Aug 15, 2023
27 checks passed
@jjerphan jjerphan deleted the maint/make-ArgKminClassMode-accept-sparse-datasets branch August 15, 2023 14:37
TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023
)

Co-authored-by: Meekail Zain <Micky774@users.noreply.github.com>
akaashpatelmns pushed a commit to akaashp2000/scikit-learn that referenced this pull request Aug 25, 2023
)

Co-authored-by: Meekail Zain <Micky774@users.noreply.github.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
)

Co-authored-by: Meekail Zain <Micky774@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:metrics Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants