PCA bug partially solved #2071
PCA bug partially solved #2071
Conversation
|
@iglesias @karlnapf Please have a look. This rectifies the PCA whitening bug. But Travis fails because of the knn data change (which will get corrected once the knn PR is in). I have tested locally, its all good. |
| @@ -172,7 +172,7 @@ bool CPCA::init(CFeatures* features) | |||
| { | |||
| for (int32_t i=0; i<num_dim; i++) | |||
| transformMatrix.col(i) /= | |||
| sqrt(eigenValues[i+max_dim_allowed-num_dim]); | |||
| sqrt(eigenValues[i+max_dim_allowed-num_dim]*(num_vectors-1)+1e-15); | |||
There was a problem hiding this comment.
why the small number here? I guess to prevent negative numbers - thats ok in principle but a bit ad-hoc.
Whats the justification? Why can eigenvalues even be negative (should not happen, and if it does, should be prevented so that we dont need this here)
There was a problem hiding this comment.
@karlnapf no its not for negative. Its to stop dividing by zero when some eigenvalues are zero (or to stop final value from blowing up when some eigen value is very very low [ie 0 theoretically]) Adding this epsilon prevents these 0/0 kind of situations during whitening. :)
There was a problem hiding this comment.
Okay, I see. Sorry I did not think about this properly before.
Ok then, do this, but make the ridge you add a class variable that one can set (not through constructor though, give it a default value). And then check whether some eigenvalues are too small, and then print a warning in which you suggest to drop that dimension via PCA.
"Covariance matrix has zero Eigenvalues at dimension %d. Consider reducing its dimension. Adding %f to avoid numerical problems"
|
Ok, nice! |
|
Thats a good idea. Let me do the suggested changes by tomorrow.
|
|
Sorry that was a mistake |
|
Nice one! Travis failed but it is not due to the changes here. I think it is my mistake actually, I merged a couple of commits to shogun-data for the nanoflann's PR (there is some changes in the KNN class) and the PR has not already been merged. |
|
All right, Travis is back to green now! @mazumdarparijat, once you address the first comment by Heiko (which should be completely straightforward for you btw) we are ready to merge :-) |
|
Lgtm! If @karlnapf agrees as well, it should be ready to merge. |
|
All right, I merge now and take the responsibility ;) What's next @mazumdarparijat? :) |
|
@iglesias Thanks!! :) |
|
Totally agree with everything :) |
|
No worries Heiko! :) |
PCA(True)doesn't work yet