Skip to content

ENH Select arpack for sparse inputs and do not raise error #28498

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 21 commits into from
Feb 29, 2024
Merged

ENH Select arpack for sparse inputs and do not raise error #28498

merged 21 commits into from
Feb 29, 2024

Conversation

lamdang2k
Copy link
Contributor

Reference Issues/PRs

Closes #28406

What does this implement/fix? Explain your changes.

  • Automatically select arpack solver when the input is sparse
  • Remove TypeError
  • Adapt test to ensure that arpack solver has been chosen

Any other comments?

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 22, 2024 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 69df867. Link to the linter CI: here

glemaitre
glemaitre reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Here are a couple of comments.

Charlie-XIAO
Charlie-XIAO reviewed Feb 25, 2024
View reviewed changes
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO 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.

Thanks for the PR @lamdang2k! Sorry for the confusion my wording may have caused you in the original issue. Here are some minor comments.

@Charlie-XIAO
Copy link
Contributor

The decomposition module heading in the changelog seems to be misplaced.

@glemaitre glemaitre self-requested a review February 27, 2024 19:48
Charlie-XIAO
Charlie-XIAO reviewed Feb 28, 2024
View reviewed changes
glemaitre
glemaitre reviewed Feb 28, 2024
View reviewed changes
Charlie-XIAO
Charlie-XIAO approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO 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 @lamdang2k!

@glemaitre glemaitre self-requested a review February 29, 2024 11:27
glemaitre
glemaitre approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@glemaitre glemaitre 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 @lamdang2k

I remove a couple of comments that were present in the code while it was already self explicit by reading the code.

@glemaitre glemaitre enabled auto-merge (squash) February 29, 2024 14:47
@glemaitre
Copy link
Member

Enabling auto-merge. Thanks @lamdang2k

@glemaitre glemaitre merged commit b369d28 into scikit-learn:main Feb 29, 2024
@lamdang2k lamdang2k deleted the select-arpack branch October 29, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@Charlie-XIAO Charlie-XIAO Charlie-XIAO approved these changes

Assignees
No one assigned
Labels
module:decomposition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PCA supports sparse now, docs suggest otherwise.
3 participants
@lamdang2k @Charlie-XIAO @glemaitre