-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
[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
Conversation
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. |
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@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(), |
There was a problem hiding this comment.
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 !
Other than the minor comments, LGTM. |
@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(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@lesteve is this fine? I'm not quite sure about the docstring though.... |
LGTM. +1 for merge. Thank you! |
LGTM too. |
No problem :) |
Rebased and squashed. |
@GaelVaroquaux @giorgiop don't know why travis is failing though :/ |
@dsquareindia you probably want to do |
Flip sign according to `u` in both cases of `transpose`.
@lesteve thanks a lot for the help! |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"flipping" => "sign flipping"
LGTM, I will address my 2 style comments myself when merging. |
Merged as cfa498c. Thanks @dsquareindia! |
Thanks for this fix @dsquareindia. It was causing me some strange surprises in dictionary learning. This nicely and cleanly resolves them. |
Flip sign according to `u` in both cases of `transpose`.
Added a Backport PR ( #6375 ) for |
Fixes #5608
Flip sign according to
u
in both cases oftranspose
.