10BC0 MAINT Make `ArgKminClassMode` accept sparse datasets by jjerphan · Pull Request #27018 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MAINT Make ArgKminClassMode accept sparse datasets #27018

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 15, 2023
Merged

MAINT Make ArgKminClassMode accept sparse datasets #27018

merged 6 commits into from
Aug 15, 2023

Conversation

@jjerphan
Copy link
Member
@jjerphan jjerphan commented Aug 5, 2023 •
edited
Loading

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?

@jjerphan
MAINT Make ArgKminClassMode accept sparse datasets
a349f97 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan jjerphan mentioned this pull request Aug 5, 2023
Merged
@github-actions
Copy link
github-actions bot commented Aug 5, 2023 •
edited
Loading

✔️ 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
thomasjpfan
thomasjpfan reviewed Aug 7, 2023
View reviewed changes
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.

Micky774
Micky774 approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor
@Micky774 Micky774 left a comment •
edited
Loading

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.

Micky774
Micky774 requested changes Aug 8, 2023
View reviewed changes
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
@jjerphan @Micky774
Override valid_metrics over is_usable_for
876f21c 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Meekail Zain <Micky774@users.noreply.github.com>
@jjerphan
docs: Add changelog entry
4200088 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan
Add a smol comma
72d1945 
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
Micky774
Micky774 approved these changes Aug 13, 2023
View reviewed changes
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 😄

@jjerphan jjerphan requested a review from thomasjpfan August 15, 2023 14:36
@thomasjpfan thomasjpfan merged commit f5c4999 into scikit-learn:main Aug 15, 2023
@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
@jjerphan @Micky774 @TamaraAtanasoska
MAINT Make ArgKminClassMode accept sparse datasets (scikit-learn#27018
5667f9a 
)

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
@jjerphan @Micky774 @akaashpatelmns
MAINT Make ArgKminClassMode accept sparse datasets (scikit-learn#27018
369b4d5 
)

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
@jjerphan @Micky774 @REDVM
MAINT Make ArgKminClassMode accept sparse datasets (scikit-learn#27018
f4757c8 
)

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

Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@Micky774 Micky774 Micky774 approved these changes

Assignees

No one assigned

Labels

module:metrics Quick Review For PRs that are quick to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jjerphan @Micky774 @thomasjpfan
0