Skip to content
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

[MRG+1] CCA low rank doesn't crash any more #4420

Merged
merged 6 commits into from
Apr 2, 2015

Conversation

amueller
Copy link
Member

Fixes #4419.
Also adds some misc shape fixes and input validation so that cross-decomposition passes the common tests.

@amueller amueller added this to the 0.16 milestone Mar 19, 2015
@amueller
Copy link
Member Author

@ogrisel this might be interesting for a back-port as it makes PLS and CCA not throw up on simple 1d data.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.01% when pulling 6bbd7c0 on amueller:cca_low_rank into bc5acea on scikit-learn:master.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.01% when pulling 3847a8c on amueller:cca_low_rank into bc5acea on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.09% when pulling 3847a8c on amueller:cca_low_rank into bc5acea on scikit-learn:master.

else:
X = np.asarray(X)
Xc -= self.x_mean_
Xc /= self.x_std_
Copy link
Member

Choose a reason for hiding this comment

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

It seems that there is a spurious indent on this line: the data is not scaled by self.x_std_ when copy=False (agreed the bug was already there prior to you PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it anyhow

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.01% when pulling 8ea474c on amueller:cca_low_rank into bc5acea on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 95.13% when pulling 8ea474c on amueller:cca_low_rank into bc5acea on scikit-learn:master.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.00% when pulling 5035b18 on amueller:cca_low_rank into bc5acea on scikit-learn:master.

@amueller
Copy link
Member Author

added tests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 95.14% when pulling 5035b18 on amueller:cca_low_rank into bc5acea on scikit-learn:master.

@ogrisel
Copy link
Member

ogrisel commented Mar 24, 2015

LGTM. +1 for merge on my side. Any quick second review? Would be great to have those fixes in 0.16.

@ogrisel ogrisel changed the title [MRG] CCA low rank doesn't crash any more [MRG+1] CCA low rank doesn't crash any more Mar 24, 2015
@amueller
Copy link
Member Author

Probably also fixes #954 but that needs to be confirmed.

@amueller
Copy link
Member Author

amueller commented Apr 2, 2015

@GaelVaroquaux maybe there is someone at the sprint to review this? ;)

@NelleV
Copy link
Member

NelleV commented Apr 2, 2015

👍

NelleV added a commit that referenced this pull request Apr 2, 2015
FIX CCA low rank doesn't crash any more
@NelleV NelleV merged commit 6c81fbc into scikit-learn:master Apr 2, 2015
@amueller
Copy link
Member Author

amueller commented Apr 2, 2015

Thanks @NelleV !

@amueller amueller deleted the cca_low_rank branch April 2, 2015 18:36
@NelleV
Copy link
Member

NelleV commented Apr 3, 2015

Well thank you!! You've not only fixed the bug but also cleaned up the code
nicely.
On Apr 2, 2015 8:36 PM, "Andreas Mueller" notifications@github.com wrote:

Thanks @NelleV https://github.com/NelleV !


Reply to this email directly or view it on GitHub
#4420 (comment)
.

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Apr 3, 2015 via email

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.

PLS not robust to rank 1 effects
6 participants