Skip to content

FIX avoid unecessary/duplicated if copy branch for sparse arrays/matrix#27336

Merged
lesteve merged 4 commits intoscikit-learn:mainfrom
glemaitre:check_array_copy_sparse
Sep 11, 2023
Merged

FIX avoid unecessary/duplicated if copy branch for sparse arrays/matrix#27336
lesteve merged 4 commits intoscikit-learn:mainfrom
glemaitre:check_array_copy_sparse

Conversation

@glemaitre
Copy link
Copy Markdown
Member

@glemaitre glemaitre commented Sep 11, 2023

This should solve the issue observed in the scipy-dev build.

#27171 (comment)

In a later PR, we should make a better test for check_array.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 11, 2023 •
edited
Loading

✔️ Linting Passed

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

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

glemaitre
glemaitre commented Sep 11, 2023
View reviewed changes
sklearn/utils/validation.py
allow_nan=force_all_finite == "allow-nan",
)

if copy:
Copy link
Copy Markdown
Member Author

@glemaitre glemaitre Sep 11, 2023

Choose a reason for hiding this comment

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

The above if take care about the SciPy sparse copy already. This should only be done in the else.

Copy link
Copy Markdown
Member

@ogrisel ogrisel Sep 11, 2023

Choose a reason for hiding this comment

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

I agree that the call to _ensure_sparse_format already takes care of handling the copy argument so there is no need to do it twice for sparse inputs.

@glemaitre glemaitre mentioned this pull request Sep 11, 2023
Merged
@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Sep 11, 2023 •
edited
Loading

The remaining failures are in the docstring and a related to the way information is displayed.

ogrisel
ogrisel reviewed Sep 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Can you please fix the title of the PR? It's seems grammatically invalid and I am not sure what you mean.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 11, 2023

I think this PR would be easier to review if it were to focus on a single problem (e.g. the check_array fix).

ogrisel
ogrisel reviewed Sep 11, 2023
View reviewed changes
@glemaitre glemaitre changed the title FIX avoid calling on sparse matrix and array in check_array FIX avoid calling np.may_share_memory on sparse arrays Sep 11, 2023
@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Sep 11, 2023

This is the latest nightly build traceback: https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=58833&view=logs&jobId=dfe99b15-50db-5d7b-b1e9-4105c42527cf&j=dfe99b15-50db-5d7b-b1e9-4105c42527cf&t=ef785ae2-496b-5b02-9f0e-07a6c3ab3081

@glemaitre glemaitre changed the title FIX avoid calling np.may_share_memory on sparse arrays FIX avoid unecessary/duplicated if copy branch for sparse arrays/matrix Sep 11, 2023
@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Sep 11, 2023

Let me revert the problem of the representation

@glemaitre glemaitre force-pushed the check_array_copy_sparse branch from 04311c2 to d65d901 Compare September 11, 2023 12:42
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Sep 11, 2023 •
edited
Loading

I kind of think this dok array issue is a scipy regression and I was hoping it would get fixed in scipy.

I was planning to follow up on scipy/scipy#18929 (comment) for directions.

Having said that, if there is an easy work-around that avoids the issue in scikit-learn, why not ...

@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Sep 11, 2023

I was planning to follow up on scipy/scipy#18929 (comment) for directions.

I open the following in scipy to kind having something working: scipy/scipy#19220

Having said that, if there is an easy work-around that avoids the issue in scikit-learn, why not ...

Actually this is a silent bug where all stars were aligned.

np.may_share_memory does not work as expected on sparse matrices. It will always returns False even if .data is shared. But we don't care of the copy here because it was taken of in the specific sparse function

ogrisel
ogrisel reviewed Sep 11, 2023
View reviewed changes
ogrisel
ogrisel approved these changes Sep 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Besides the previous comments, the fix itself LGTM!

@glemaitre @ogrisel
Update sklearn/utils/tests/test_validation.py
7458957 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
lesteve
lesteve approved these changes Sep 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

LGTM, putting this on auto-merge

@lesteve lesteve enabled auto-merge (squash) September 11, 2023 13:46
@lesteve lesteve merged commit e7ae63f into scikit-learn:main Sep 11, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@glemaitre @ogrisel @REDVM
FIX avoid unecessary if copy branch for sparse array/matrix (scikit…
28a7f31 
…-learn#27336)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogrisel ogrisel ogrisel approved these changes

@lesteve lesteve lesteve approved these changes

Assignees

No one assigned

Labels

module:utils

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@glemaitre @ogrisel @lesteve