-
-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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
MAINT Make ArgKminClassMode
accept sparse datasets
#27018
Conversation
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
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.
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.
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.
LGTM pending changelog entry
Actually we can avoid that by specifying |
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.
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.
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>
8eb58b0
to
72d1945
Compare
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.
LGTM, thanks 😄
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
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 overloadsis_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?