[MRG+2] Making Spectral Embedding Deterministic by Hasil-Sharma · Pull Request #4249 · scikit-learn/scikit-learn

Hasil-Sharma · 2015-02-15T17:04:35Z

fixes #4236

raghavrv · 2015-02-15T17:11:13Z

Thanks for the PR...

3 minor points :

Could you squash all commits to 1?
Please use WIP / MRG as tags in front of PR title...
In the PR description, add "fixes Make spectral_embedding deterministic #4236" to automatically close the issue after this gets merged in

raghavrv · 2015-02-15T17:11:59Z

sklearn/manifold/spectral_embedding_.py

axis = 1 --> axis=1

alternatively you could run flake8 to check for such issues :)

coveralls · 2015-02-15T17:15:57Z

Coverage increased (+0.0%) to 95.04% when pulling b5f91f2 on Hasil-Sharma:my-feature into 5f95154 on scikit-learn:master.

Hasil-Sharma · 2015-02-15T17:16:00Z

Thanks for reviewing my PR.
I'll do all that you have stated above and would send a new PR.

raghavrv · 2015-02-15T17:16:42Z

sklearn/manifold/tests/test_spectral_embedding.py

This argument is not needed...

raghavrv · 2015-02-15T17:23:12Z

No no please don't send a new pr... do the following :

Just reset your work - git reset HEAD~<NO_OF_COMMITS_MADE_BY_U>
Make the changes
Recommit
Force push to the same branch - git push -u -f my-feature:my-feature

It will get updated here... :)

Hasil-Sharma · 2015-02-15T18:01:26Z

@ragv : Thanks for assisting me so patiently, I have done all that you had mentioned in one of your comments. Please tell if it looks good and if there is anything else that I should do ?

raghavrv · 2015-02-15T18:07:17Z

You seemed to have reset a few commits that was not made for this PR and added it to a single commit "Making spec..."

Please do a git rebase master to fix that...

raghavrv · 2015-02-15T18:10:23Z

after the rebase simply do a force push...

Hasil-Sharma · 2015-02-15T18:14:30Z

Yes, between the commits I had done I have merged the master branch with my-feature to update the my-feature branch. Would git rebase master resolve the issue ?

raghavrv · 2015-02-15T18:16:22Z

yea :)

raghavrv · 2015-02-15T18:17:10Z

or wait...

raghavrv · 2015-02-15T18:19:15Z

do:

git rebase -i HEAD~<COMMIT_COUNT_FOR_THIS_PR+1(for merge commit)>
delete the line that has the merge commit
save and close the file
now do a git rebase master

And in general do a git rebase master to update your branch

Hasil-Sharma · 2015-02-15T19:11:27Z

@ragv : Thanks for assisting me, I think PR is now as it is should be ?

raghavrv · 2015-02-15T19:37:41Z

LGTM 👍

raghavrv · 2015-02-15T19:47:13Z

Also can you add test for eigen_sign_flip ? ( to be added to test_spectral_embedding.py )

Hasil-Sharma · 2015-02-15T20:06:52Z

Thanks :)

Yeah, how should I go about about ?

Creating a random n-array on my own and saving it's correct answer and later checking if random n-array is same as correct answer or simply checking that sign of largest absolute value in array in positive or some way else ?

raghavrv · 2015-02-15T20:28:40Z

How about both ;)

Take an unflipped ( 5 x 5 would be sufficient I think )
Check if that array, before flipped, does not have +ve sign for the largest abs val
Do flip
Check with hardcoded value
Check if now, all largest abs are +ve

raghavrv · 2015-02-16T15:30:05Z

sklearn/manifold/tests/test_spectral_embedding.py

Could you pl. convert this to a comment? ( Related to #4250 )

( also do the same for the test for eigen_sign_flip )

@ragv : This is what I have done, please tell if it would be enough ?

Finding the indexes of maximum absolute value in all the vectors.

Flipping the vector signs by running _eigen_sign_flip

Finding the indexes of maximum value in all the vectors.

Asserting that indexes in step 1 and step 3 are same by using assert_array_equal

Yes this is sufficient, I guess

ogrisel · 2015-02-17T08:34:52Z

@Hasil-Sharma once you have addressed the comments please rename the PR title from [WIP] to [MRG] to make it explicit that this is ready for final review.

raghavrv · 2015-02-23T20:34:11Z

I'll change [WIP] to [MRG]

Yea sure! :)

BTW could I trouble you with ~~two~~ a few more minor fixes to your code ? :p

Hasil-Sharma · 2015-02-23T20:39:59Z

Pray tell me minor fixes I should make :)

raghavrv · 2015-02-23T21:08:05Z

sklearn/utils/tests/test_extmath.py

combine both lines to get
data = np.random.RandomState(36).randn(5, 5)

~~or if you feel like, could you instead refactor all these out ( since they are repeated across multiple tests ) to the top like~~
~~rng = np.random.RandomState(0)~~
~~and use rng everywhere inside the tests instead? ( preferably in a separate commit )...~~
~~like rng.randn(5, 5) etc...~~ Ignore this... This could perhaps be done in a separate PR...

raghavrv · 2015-02-23T21:40:18Z

After the above 4 fixes please change from [WIP] to [MRG+1] ! Thanks for working on this!

Hasil-Sharma · 2015-02-24T07:27:23Z

@ragv : All changes done, and changing [WIP] to [MRG+1] Thanks for guiding me all the way through :)

ogrisel · 2015-02-24T14:27:48Z

sklearn/utils/extmath.py

I would rename this to _deterministic_vector_sign_flip (the _ to keep it a private utility function). This function does not need to work only with eigen vectors of a matrix but any vectors so there is no need to put 'eigen' in the name or docstring.

amueller · 2015-02-24T23:58:30Z

After renaming and removing references to this being eigenvectors, +1 for merge.

Hasil-Sharma · 2015-02-25T08:49:14Z

All changes done :)

amueller · 2015-02-25T18:53:51Z

LGTM

ogrisel · 2015-02-26T08:07:39Z

sklearn/utils/tests/test_extmath.py

I will do the changes myself when merging but FYI PEP8 requires 2 blank lines to separate top level blocks and white spaces around binary operators.

ogrisel · 2015-02-26T08:16:18Z

Merged by rebase, thanks for your contribution @Hasil-Sharma!

amueller · 2015-02-26T14:26:47Z

Hum, for some reason, adding SpectralEmbedding to the test still fails:
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/tests/test_common.py#L107
Maybe that should have been part of the PR.

raghavrv · 2015-02-26T19:35:52Z

I think this happens since, for the particular input setup, the graph is not fully connected as it warns accordingly... using affinity="rbf" fixes this up... Could you confirm if that fix is correct?

amueller · 2015-02-26T19:43:10Z

but shouldn't it still be deterministic?

raghavrv · 2015-02-26T19:49:38Z

I am sorry I do not understand how it works, but from the warning message -

Graph is not fully connected, spectral embedding may not work as expected.

does that mean it can be expected to be non deterministic for input setups which gives a not-fully-connected adj. matrix?

amueller · 2015-02-26T19:53:27Z

ok but then we should investigate why that is.

Hasil-Sharma changed the title ~~Making Spectral Embedding Deterministic #4236~~ Making Spectral Embedding Deterministic Feb 15, 2015

Hasil-Sharma mentioned this pull request Feb 15, 2015

Make spectral_embedding deterministic #4236

Closed

raghavrv reviewed Feb 15, 2015
View reviewed changes

sklearn/manifold/tests/test_spectral_embedding.py Outdated

Copy link
Copy Markdown

Member

raghavrv Feb 15, 2015

Choose a reason for hiding this comment

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

This argument is not needed...

Hasil-Sharma force-pushed the my-feature branch from b5f91f2 to c413c0e Compare February 15, 2015 17:51

Hasil-Sharma changed the title ~~Making Spectral Embedding Deterministic~~ [WIP]Making Spectral Embedding Deterministic Feb 15, 2015

Hasil-Sharma force-pushed the my-feature branch from c413c0e to 2c53c7c Compare February 15, 2015 19:08

raghavrv reviewed Feb 16, 2015
View reviewed changes

Hasil-Sharma force-pushed the my-feature branch from 2c53c7c to 37f276a Compare February 17, 2015 11:36

raghavrv reviewed Feb 23, 2015
View reviewed changes

Hasil-Sharma force-pushed the my-feature branch from f67b00d to 226d905 Compare February 24, 2015 07:26

Hasil-Sharma changed the title ~~[WIP]Making Spectral Embedding Deterministic~~ [MRG+1]Making Spectral Embedding Deterministic Feb 24, 2015

ogrisel reviewed Feb 24, 2015
View reviewed changes

Hasil-Sharma force-pushed the my-feature branch from 226d905 to a25e86f Compare February 25, 2015 08:47

Making Spectral Embedding Deterministic

72a6e08

Hasil-Sharma force-pushed the my-feature branch from a25e86f to 72a6e08 Compare February 25, 2015 08:50

amueller changed the title ~~[MRG+1]Making Spectral Embedding Deterministic~~ [MRG+2] Making Spectral Embedding Deterministic Feb 25, 2015

ogrisel reviewed Feb 26, 2015
View reviewed changes

ogrisel closed this Feb 26, 2015

raghavrv mentioned this pull request Feb 26, 2015

[WIP] Make spectral embedding unconditionally deterministic and related fixes. #4299

Closed

5 tasks

Hasil-Sharma deleted the my-feature branch February 28, 2015 08:53

Hasil-Sharma restored the my-feature branch February 28, 2015 09:03

Uh oh!

Conversation

Hasil-Sharma commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 15, 2015

Uh oh!

Hasil-Sharma commented Feb 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

Hasil-Sharma commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

Hasil-Sharma commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

Hasil-Sharma commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

Hasil-Sharma commented Feb 15, 2015

Uh oh!

raghavrv commented Feb 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 17, 2015

Uh oh!

raghavrv commented Feb 23, 2015

Uh oh!

Hasil-Sharma commented Feb 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghavrv commented Feb 23, 2015

Uh oh!

Hasil-Sharma commented Feb 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Feb 24, 2015

Uh oh!

Hasil-Sharma commented Feb 25, 2015

Uh oh!

amueller commented Feb 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 26, 2015

Uh oh!

amueller commented Feb 26, 2015

Uh oh!

raghavrv commented Feb 26, 2015

Uh oh!

amueller commented Feb 26, 2015

Uh oh!