8000 ENH: make consistent interface for scipy's eigs, eigsh and svds with random start · Issue #5545 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

ENH: make consistent interface for scipy's eigs, eigsh and svds with random start #5545

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
giorgiop opened this issue Oct 23, 2015 · 15 comments · Fixed by #18302
Closed

ENH: make consistent interface for scipy's eigs, eigsh and svds with random start #5545

giorgiop opened this issue Oct 23, 2015 · 15 comments · Fixed by #18302
Assignees
@gauravkdesai
Labels
Easy Well-defined and straightforward way to resolve Enhancement good first issue Easy with clear instructions to resolve

Comments

@giorgiop
Copy link
Contributor

Recent PRs #5012 #5243 introduce random seeding for those scipy calls to ARPACK, that are otherwise non-deterministic. We should make them consistent across the code base. Somewhere in sklearn.utils is the place.

The interface should also be extended to every call we make to those functions in sklearn.
@giorgiop
Copy link
Contributor Author

Ping @yanlend in case you would like to take care of this. #5243 is yet to be merged though.
Ping @ogrisel for discussion.

@giorgiop giorgiop changed the title ENH: make consistent interface for scipy's eig, eigs and svds with random start ENH: make consistent interface for scipy's eigs, eigsh and svds with random start Oct 23, 2015
@giorgiop giorgiop mentioned this issue Oct 24, 2015
Open
@ogrisel
Copy link
Member
ogrisel commented Jul 5, 2018 •
edited
Loading

Sounds like a good idea and potentially a good task for a first-time contributor to scikit-learn.

If you are interested, please first try to fully read the diffs and the discussions in the #5012 PR and the scipy documentation on eigs, eighs and svds to understand the context and get familiar with the relevant part of the code base.

Then use commands such as git grep eigs, git grep eigsh, git grep svds and git grep "v0=" to find all the parts of the scikit-learn code base that could benefit from factorizing that initialization strategy in a utility function such as sklearn.utils._init_arpack_v0(size, random_state) that could be used to generate the v0 vector correctly whenever we use the eigsh and svds solvers in scikit-learn.

It's important to also list the uses of eigs, eigsh and svds that currently do not use deterministic seeding by passing the v0 option.

@ogrisel ogrisel added Easy Well-defined and straightforward way to resolve Enhancement help wanted good first issue Easy with clear instructions to resolve labels Jul 5, 2018
@ogrisel
Copy link
Member
ogrisel commented Jul 5, 2018

Also, it might be interesting to also contact discuss with upstream scipy on whether or not they would like to add an optional parameter named seed or random_state that would be used to initialize v0 when v0 is not explicitly passed by the caller.

@summer-bebop
Copy link
Contributor

Hi. Since it's a good task for a first timer I would be glad to follow the guidelines you provided and tackle this issue. Would it be ok ?

@ogrisel
Copy link
Member
ogrisel commented Jul 5, 2018

Sure: make sure you understand everything I mentioned above, no need to ask for permission.

@ogrisel
Copy link
Member
ogrisel commented Jul 5, 2018

Also read about: http://scikit-learn.org/dev/developers/contributing.html#random-numbers

This was referenced Jul 11, 2018
Open
Closed
@imnotaqtpie
Copy link

hi @ogrisel this seems like a good place to get started and since its been over a year since the last activity, is it ok for me to take this up?

Uh oh!

There was an error while loading. Please reload this page.
@jeremiedbb
Copy link
Member

Most of the work has been done in #11524 besides a couple comments. You can take over his work (I can speak for him :) ). I advise you to read the discussion in #11524 to see why some choices have been made and how to resolve the comments that have not been addressed.

@summer-bebop
Copy link
Contributor
summer-bebop commented Nov 29, 2019 via email

@imnotaqtpie
Copy link

sure thing, ill take a look at the comments and finish up what i can.
thanks

@gauravkdesai
Copy link
Contributor

@ogrisel @jeremiedbb @FollowKenny does this issue needs any help? I would like this to be my first contribution if possible.

@jeremiedbb
Copy link
Member

Thanks @gauravkdesai. You can take over #11524 which is stalled but almost ready. You'd probably have to fix some merge conflicts. There are also a few comments left to address.

@gauravkdesai
Copy link
Contributor

@jeremiedbb thanks.

@gauravkdesai
Copy link
Contributor

take

@gauravkdesai gauravkdesai mentioned this issue Aug 30, 2020
Merged
@giorgiop
Copy link
Contributor Author

WOW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gauravkdesai gauravkdesai

Labels
Easy Well-defined and straightforward way to resolve Enhancement good first issue Easy with clear instructions to resolve
Projects
None yet
Milestone
No milestone
6 participants
@ogrisel @giorgiop @summer-bebop @jeremiedbb @gauravkdesai @imnotaqtpie
0