-
-
Notifications
You must be signed in to change notification settings - Fork 26k
[MRG] KernelPCA: fix transform issue when zero eigenvalues are present and not removed (issue 12141) #12143
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] KernelPCA: fix transform issue when zero eigenvalues are present and not removed (issue 12141) #12143
Conversation
- Added a few comments to clarify `_fit_transform`, `fit_transform` and `transform`. - Uniformized the numpy coding style for `fit` and `fit_transform`.
493e7b5
to
27b4de5
Compare
All set, waiting for review. |
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.
…s where there is no zero division, to avoid numpy warnings.
…o zero division numpy warning.
…ture to perform fine-grain warning assertion
Thanks @smarie , LGTM. Let's wait for other reviews now. |
Thanks @NicolasHug . If you have still a couple time for reviews in the next days, I would really appreciate a review for the companion PRs of this PR:
|
Your #12069 (comment)
makes me think we should not test for strict equality with zero but instead have an approximate comparison. |
Actually that's already taken care of - but in #12145 :) where we round to zero the small eigenvalues that are due to bad numerical conditioning (as well as negative eigenvalues because a kernel is supposed to be semi-positive definite so they are probably numerical errors). See https://github.com/scikit-learn/scikit-learn/pull/12145/files#diff-3b70045c110a30d29de66ed0ea3fb86dR1082 for details |
OK. I guess this would make sense to do it here? IMHO the way to go would be to first merge this one, then #12145, and then #12069. This way you can merge master each time to avoid the redundant changes in the diffs (right now #12145 diffs also show the changes from here and that make reviewing trickier) |
Yes, this is the merge order that should be used since each PR contains and relies on the commits from the previous. I am not sure that it would make sense to backport the zero eigenvalues rounding here: indeed, that is precisely what #12145 is about : controling and harmonizing the eigenvalues found by the various solvers. |
You're right, if #12145 sets small eigenvalues to 0 then the code here won't have to change. |
Hi @NicolasHug , @jnothman , @ogrisel : any update on this PR and the subsequent ones #12145 and #12069 ? Experience shows that the more we wait the more likely the PRs will not be mergeable again... Of course I understand that this is a volunteer-based open-source project with many contributions, but I really believe that the effort we collectively made on these three PR both in terms of idea (@grilling original code in matlab), coding, and code review, is mature now. |
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.
Nitpicks from me, this still LGTM.
I find the comments that you added very helpful.
Hope we can get other reviewers on this!
Maybe @adrinjalali and @qinhanmin2014 could to review this? Should be quick to review IMO (LGTM already) and it'd be nice to address @smarie great efforts ;) |
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.
Few nits about the comments. Otherwise LGTM
Co-Authored-By: smarie <sylvain.marie@schneider-electric.com>
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.
Thanks @smarie
This is +2 now, @thomasjpfan can you merge it please? |
Thanks everyone! |
…resent and not removed (scikit-learn#12143)" This reverts commit ba2bb79.
…resent and not removed (scikit-learn#12143)" This reverts commit ba2bb79.
This fixes #12141