-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
TST Extend tests for scipy.sparse.*array
in sklearn/feature_selection/tests/test_feature_select.py
#27239
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
Conversation
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 @Tialo
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.
Actually, the implementation of r_regression
is not compatible. I have to check the reason.
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.
You need to edit the file sklearn/feature_selection/_univariate_selection.py
in l.326 and replace the code from:
if issparse(X):
X_means = X.mean(axis=0).getA1()
else:
X_means = X.mean(axis=0)
to
if isspmatrix(X):
X_means = X.mean(axis=0).getA1()
else:
X_means = X.mean(axis=0)
You should import isspmatrix
from scipy.sparse
a18e73c
to
180013a
Compare
Thanks for fix! |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
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.
I fixed the linting problem by removing the useless import. I also added an entry in the changelog at the same time since we are modifying some public facing code.
LGTM otherwise. @OmarManzoor feel free to have a look at this one.
doc/whats_new/v1.4.rst
Outdated
|
||
- |Enhancement| :func:`metrics.f_regression` and :func:`metrics.r_regression` now | ||
supports SciPy sparse arrays. | ||
:pr:`27239` by :user:`Tialo <Tialo>`. |
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.
Feel free to update your name (only the first mention since the second one link to your GitHub handle). This is also fine keeping it as-is.
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.
Thanks @Tialo for the PR.
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.
I made the minor changes. LGTM.
Thanks! I changed my name, ready to merge |
Oh apparently there is a new deprecation warning. I made the changes. We should be good to go. |
…ion/tests/test_feature_select.py` (scikit-learn#27239) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Reference Issues/PRs
Towards #27090.
What does this implement/fix? Explain your changes.
Any other comments?