[MRG+1] FIX #5608 in randomized_svd flip sign#6135
[MRG+1] FIX #5608 in randomized_svd flip sign#6135devashishd12 wants to merge 1 commit intoscikit-learn:masterfrom devashishd12:svd_flip_fix
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. |
sklearn/utils/tests/test_extmath.py
Outdated
There was a problem hiding this comment.
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.
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. |
sklearn/utils/tests/test_extmath.py
Outdated
There was a problem hiding this comment.
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? |
sklearn/utils/tests/test_extmath.py
Outdated
There was a problem hiding this comment.
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_basedYou could also put your comment as the max_loading_is_positive function docstring.
There was a problem hiding this comment.
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? |
|
|
||
|
|
||
| def test_randomized_svd_sign_flip_with_transpose(): | ||
| # Check if randomized_svd flipping is always done based on u |
|
LGTM, I will address my 2 style comments myself when merging. |
|
Merged as cfa498c. Thanks @dsquareindia! |
Fixes #5608
Flip sign according to
uin both cases oftranspose.