10000 Implement `make_sparse_spd_matrix` using a sparse memory layout from the start · Issue #27433 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Implement make_sparse_spd_matrix using a sparse memory layout from the start #27433

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
Closed
ogrisel opened this issue Sep 21, 2023 · 9 comments
Closed

Implement make_sparse_spd_matrix using a sparse memory layout from the start #27433

ogrisel opened this issue Sep 21, 2023 · 9 comments
Assignees
@Charlie-XIAO
Labels
Moderate Anything that requires some knowledge of conventions and best practices New Feature

Comments

@ogrisel
Copy link
Member
ogrisel commented Sep 21, 2023 •
edited
Loading

Describe the workflow you want to enable

As discussed in #27359, make_sparse_spd_matrix actually returns a dense numpy array (with many zero values).

I think it should be possible to rewrite this code to compose operations on sparse arrays/matrices from the start instead of allocating large dense square matrices.

I have not tried myself, but I think it should be doable and it should be much more memory efficient.
@glemaitre glemaitre mentioned this issue Sep 21, 2023
Closed
@adrinjalali adrinjalali added the Moderate Anything that requires some knowledge of conventions and best practices label Sep 21, 2023
@Charlie-XIAO
Copy link
Contributor

Can I try this one?

@adrinjalali
Copy link
Member

That'd be nice @Charlie-XIAO

@Charlie-XIAO
Copy link
Contributor

Surprisingly, this function does not seem to be tested at all (if I have not missed anything).

@Charlie-XIAO
Copy link
Contributor

/take

@Charlie-XIAO Charlie-XIAO mentioned this issue Sep 22, 2023
Merged
@StefanieSenger
Copy link
Contributor

Surprisingly, this function does not seem to be tested at all (if I have not missed anything).

It's implicitly used in test_graphical_lasso.py and in test_extmath.py, but it should actually be tested in test_samples_generator.py.

@Charlie-XIAO
Copy link
Contributor
Charlie-XIAO commented Sep 22, 2023 •
edited
Loading

True, I'm also seeing those from the CI. It is, however, tricky to test its parameters which I explained in the linked PR.

@StefanieSenger
Copy link
Contributor

Sorry, @Charlie-XIAO, I have overseen that you've already addressed that with the PR.

@Charlie-XIAO
Copy link
Contributor

Please don't apologize @StefanieSenger, that doesn't matter at all. I just wanted to say that if you are interested, you can look at my analysis in that PR. In fact I have just seen that CI is complaining about test_extmath.py when you posted the comment.

@Charlie-XIAO
Copy link
Contributor

@ogrisel I think we can close this one. Sorry that I forgot to write Fixes #xxx in the PR so this was not automatically done.

@ogrisel ogrisel closed this as completed Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Charlie-XIAO Charlie-XIAO

Labels
Moderate Anything that requires some knowledge of conventions and best practices New Feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ogrisel @adrinjalali @StefanieSenger @Charlie-XIAO
0