Skip to content

FIX: spectral_embedding.py initial eigenvector approximations for LOBPCG and AMG solvers should be randn not rand #21565

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 5 commits into from
Dec 14, 2021
Merged

FIX: spectral_embedding.py initial eigenvector approximations for LOBPCG and AMG solvers should be randn not rand #21565

merged 5 commits into from
Dec 14, 2021

Conversation

lobpcg
Copy link
Contributor

@lobpcg lobpcg commented Nov 5, 2021 •
edited
Loading

rand -> randn for LOBPCG and AMG eigensolvers

Reference Issues/PRs

Fixes #21555. See also #21534

@lobpcg
Update _spectral_embedding.py
77f7af1 
rand -> randn for LOBPCG and AMG eigensolvers
@lobpcg lobpcg changed the title spectral_embedding.py initial eigenvector approximations for LOBPCG and AMG solvers should be randn not rand FIX: spectral_embedding.py initial eigenvector approximations for LOBPCG and AMG solvers should be randn not rand Nov 5, 2021
@ogrisel
Copy link
Member

ogrisel commented Nov 5, 2021

Could you please add a non-regression test with a toy dataset where the uniform sampling would lead to bad results while the gaussian is good?

@lobpcg
Copy link
Contributor Author

lobpcg commented Nov 5, 2021 •
edited
Loading

Could you please add a non-regression test with a toy dataset where the uniform sampling would lead to bad results while the gaussian is good?

@ogrisel It's not trivial since it's random. It would require a fixed seed and would be platform-dependent, as most effects of numerical instability are.

It's a common understanding that randn should be used here, not rand; see my notes in #21555

In the previous PR we were going just to merge it. Can we do it?

@lobpcg
Copy link
Contributor Author

lobpcg commented Nov 19, 2021

@ogrisel could you please merge this trivial change?

@lobpcg
Copy link
Contributor Author

lobpcg commented Dec 3, 2021

@ogrisel or anyone else - could we please just merge this trivial change?

thomasjpfan
thomasjpfan reviewed Dec 3, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan 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.

If it is in the original paper, then I think we can considered this PR a bug fix. Can you point to the section in the paper that specifies the initial distribution? (I am unable to find it)

lobpcg added 2 commits December 3, 2021 16:01
@lobpcg
Update v1.1.rst
1c7e325 
added 2 changed models as requested
@lobpcg
Merge branch 'main' into patch-1
67c3afd
thomasjpfan
thomasjpfan reviewed Dec 4, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good to me. I left some comments about the "Change Model" section.

@lobpcg
Update v1.1.rst
37ac937 
accepted proposed editing
@lobpcg
Copy link
Contributor Author

lobpcg commented Dec 4, 2021

@thomasjpfan - all fixed, thanks again for your help! Ready to merge?

thomasjpfan
thomasjpfan approved these changes Dec 5, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@lobpcg
Copy link
Contributor Author

lobpcg commented Dec 5, 2021

Ready to merge, I suppose.

@lobpcg lobpcg mentioned this pull request Dec 6, 2021
Merged
@lobpcg
Copy link
Contributor Author

lobpcg commented Dec 8, 2021

The corresponding change in lobpcg documentation has been merged in scipy/scipy#15154

This PR is ready to merge IMHO

@thomasjpfan
Copy link
Member

Since this PR could result in a model change, I think this PR requires a second review. Maybe @ogrisel ?

@lobpcg
Copy link
Contributor Author

lobpcg commented Dec 14, 2021

@ogrisel or anyone else - could you please review and merge this trivial change, while there is no conflict yet?

ogrisel
ogrisel approved these changes Dec 14, 2021
View reviewed changes
Copy link
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.

LGTM, sorry for the slow response time.

@ogrisel ogrisel merged commit 08e7b7c into scikit-learn:main Dec 14, 2021
@lobpcg lobpcg deleted the patch-1 branch December 14, 2021 16:05
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

@thomasjpfan thomasjpfan thomasjpfan approved these changes

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

Successfully merging this pull request may close these issues.

spectral_embedding.py initial eigenvector approximations for LOBPCG and AMG solvers should be randn not rand
3 participants
@lobpcg @ogrisel @thomasjpfan