Skip to content

[MRG+1] FIX #5608 in randomized_svd flip sign #6135

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.

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

Closed
wants to merge 1 commit into from
Closed

[MRG+1] FIX #5608 in randomized_svd flip sign #6135

wants to merge 1 commit into from

Conversation

devashishd12
Copy link
Contributor

Fixes #5608
Flip sign according to u in both cases of transpose.

@lesteve
Copy link
Member

lesteve commented Jan 8, 2016

Github tip: a good practice is to put fix #issue_number in the description of the issue. This way Github will close the associated issue automatically when the PR is merged.

About your error: "TabError: inconsistent use of tabs and spaces in indentation", that probably means that you switch to an editor with better support for editing python. Also you should invest some time setting up flake8 checks in your editor so that these errors can be spotted while editing the code rather than when running the tests.

About the fix itself, lines 364-367 have a mix of tabs and spaces you should fix them by using spaces only.

Just adding #5608 so that this adds a link to the associated issue.

@lesteve
Copy link
Member

lesteve commented Jan 8, 2016

Also small scikit-learn tip: put [MRG] at the beginning of your PR title when you feel your PR is ready to be merged.


# With transpose
u_flipped_with_transpose, _, v_flipped_with_transpose = randomized_svd(
mat, 3, flip_sign=True,
Copy link
Member

Choose a reason for hiding this comment

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

you have indentation mis-alignment here too . Setting up flake8 checks in your editor would help spotting those too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very sorry for that. I set it up now. Will address the issue right away

@devashishd12 devashishd12 changed the title FIX #5608 in randomized_svd flip sign [MRG] FIX #5608 in randomized_svd flip sign Jan 8, 2016
@devashishd12
Copy link
Contributor Author

@lesteve thanks a lot! I'll keep all that in mind next time.

# Check if flipping is always done based on u
def max_loading_is_positive(u, v):
# u_based, v_based
return((np.abs(u).max(axis=0) == u.max(axis=0)).all(),
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: a space after a return seems more usual.

Maybe add a comment explaining what is going on in this computation. It is not so easy to grok even if I may have written it originally, so imagine what it will be like in a few months.

My personal attempt: the values maximising np.abs over the rows (resp. columns) should all be positive for u (resp. v). Not that great so try to improve it !

@lesteve
Copy link
Member

lesteve commented Jan 8, 2016

Other than the minor comments, LGTM.

@devashishd12
Copy link
Contributor Author

@lesteve could you please check once?

# the values maximising np.abs
# are positive across all rows
# for u and across all columns for v.
return((np.abs(u).max(axis=0) == u.max(axis=0)).all(),
Copy link
Member

Choose a reason for hiding this comment

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

Just for readability I would do something like:

u_based = (np.abs(u).max(axis=0) == u.max(axis=0)).all()
v_based = (np.abs(v).max(axis=1) == v.max(axis=1)).all()

return u_based, v_based

You could also put your comment as the max_loading_is_positive function docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I include lines for parameters, return value etc or should I just enclose the comment within triple quotes?

@devashishd12
Copy link
Contributor Author

@lesteve is this fine? I'm not quite sure about the docstring though....

@GaelVaroquaux GaelVaroquaux changed the title [MRG] FIX #5608 in randomized_svd flip sign [MRG+1] FIX #5608 in randomized_svd flip sign Jan 10, 2016
@GaelVaroquaux
Copy link
Member

LGTM. +1 for merge. Thank you!

@giorgiop
Copy link
Contributor

LGTM too.

@devashishd12
Copy link
Contributor Author

No problem :)

@devashishd12
Copy link
Contributor Author

Rebased and squashed.

@devashishd12
Copy link
Contributor Author

@GaelVaroquaux @giorgiop don't know why travis is failing though :/

@lesteve
Copy link
Member

lesteve commented Jan 21, 2016

@dsquareindia you probably want to do git commit --amend and force push to trigger a new CI build. Looks like one of the Travis build got stuck for a reason that doesn't have to do anything with this PR.

Flip sign according to `u` in both cases of `transpose`.
@devashishd12
Copy link
Contributor Author

@lesteve thanks a lot for the help!

@devashishd12
Copy link
Contributor Author

@GaelVaroquaux @amueller ping. Can this be merged?

@@ -330,6 +331,39 @@ def test_randomized_svd_sign_flip():
assert_almost_equal(np.dot(v2.T, v2), np.eye(2))


def test_randomized_svd_sign_flip_with_transpose():
# Check if randomized_svd flipping is always done based on u
Copy link
Member

Choose a reason for hiding this comment

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

"flipping" => "sign flipping"

@ogrisel
Copy link
Member

ogrisel commented Feb 8, 2016

LGTM, I will address my 2 style comments myself when merging.

@ogrisel
Copy link
Member

ogrisel commented Feb 8, 2016

Merged as cfa498c. Thanks @dsquareindia!

@ogrisel ogrisel closed this Feb 8, 2016
@devashishd12
Copy link
Contributor Author

No problem @ogrisel! Thanks a lot @lesteve for the help!

@jakirkham
Copy link
Contributor

Thanks for this fix @dsquareindia. It was causing me some strange surprises in dictionary learning. This nicely and cleanly resolves them.

jakirkham referenced this pull request Feb 17, 2016
Flip sign according to `u` in both cases of `transpose`.
@jakirkham
Copy link
Contributor

Added a Backport PR ( #6375 ) for 0.17.X.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants