8000 Making partial_svd and tucker reproducible by lan496 · Pull Request #139 · tensorly/tensorly · GitHub
[go: up one dir, main page]
Skip to content

Making partial_svd and tucker reproducible #139

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
Nov 20, 2019
Merged

Making partial_svd and tucker reproducible #139

merged 5 commits into from
Nov 20, 2019

Conversation

lan496
Copy link
Contributor
@lan496 lan496 commented Nov 15, 2019 •
edited
Loading

fix #138

  • I add the option of a starting vector for scipy.sparse.linalg.eigsh in tensorly.partial_SVD
  • I also append a test checking if the same result for partial SVD is obtained.

I've checked my commit passes all the tests by make test.

@coveralls
Copy link
coveralls commented Nov 15, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 95.541% when pulling 2a38abf on lan496:reproducibility-for-tucker into f1a7bf2 on tensorly:master.

@lan496 lan496 changed the title option for starting vector in partial svd [WIP]option for starting vector in partial svd Nov 15, 2019
@lan496
Copy link
Contributor Author
lan496 commented Nov 15, 2019 •
edited
Loading

Sorry, this fix does not seem to be compatible with backends other than numpy.
I'll handle it.

@lan496 lan496 changed the title [WIP]option for starting vector in partial svd Making partial_svd and tucker reproducible Nov 16, 2019
@lan496
Copy link
Contributor Author
lan496 commented Nov 16, 2019

I've handled multiple backends.
And I've changed the code of Tucker decomposition to be deterministic with random_state.

@JeanKossaifi
Copy link
Member

Hi @lan496,
Thanks for the issue and PR to fix it!

Sorry, this fix does not seem to be compatible with backends other than numpy.
I'll handle it.

I like the attitude! :D

The fix looks great. One thing: rather than inspecting the signature of the SVD fun and creating a partial version, we could just add a **kwargs in all and use random_state when relevant?

@lan496
Copy link
Contributor Author
lan496 commented Nov 19, 2019

Thank you for your suggestion, @JeanKossaifi .
I add kwargs for all the SVD functions: That's more flexible!

@JeanKossaifi
Copy link
Member

Thanks @lan496!

Just a comment regarding the initialization of v0: in general initializing correctly these algorithms is not trivial:

  • If random_state is None, I would initialize v0 to None: it is better to let Arpack choose the init.
  • I also do not think that rand is the most appropriate choice, see discussion in scikit-learn and doc of Arpack. We should probably at least use a uniform distribution in [-1, 1].

Looks good otherwise!

@lan496
Copy link
Contributor Author
lan496 commented Nov 20, 2019

I'm done.
Honestly, I wasn't confident with how to initialize v0. So thank you for sharing the related discussion and docs.

@JeanKossaifi
Copy link
Member

Awesome, thanks @lan496, merging!

@JeanKossaifi JeanKossaifi merged commit dc67ac6 into tensorly:master Nov 20, 2019
@lan496 lan496 deleted the reproducibility-for-tucker branch November 20, 2019 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option for starting vector in partial SVD
3 participants
@lan496 @coveralls @JeanKossaifi
0