Moving EVD and SVD heavy lifiting into separate methods #3775
Conversation
| @@ -102,168 +102,176 @@ bool CPCA::init(CFeatures* features) | |||
|
|
|||
| // center data | |||
| Map<MatrixXd> fmatrix(feature_matrix.matrix, num_features, num_vectors); | |||
|
|
|||
There was a problem hiding this comment.
you can put your name to the authors at the beginning of the file
as well as changing the license to BSD (grep for BSD license in the source tree)
src/shogun/preprocessor/PCA.cpp
Outdated
|
|
||
| if (m_whitening) { | ||
| for (int32_t i = 0; i < num_dim; i++) { | ||
| if (CMath::fequals_abs<float64_t>(0.0, eigenValues[i], m_eigenvalue_zero_tolerance)) { |
There was a problem hiding this comment.
minor style:
We do new lines for {
|
|
||
| // covariance matrix | ||
| MatrixXd cov_mat(num_features, num_features); | ||
| cov_mat = fmatrix*fmatrix.transpose(); |
There was a problem hiding this comment.
next (small) PR: transpose actually changes the matrix memory, which is horrible. adjoint is the method we want to use here
src/shogun/preprocessor/PCA.h
Outdated
| @@ -225,6 +225,10 @@ class CPCA: public CDimensionReductionPreprocessor | |||
| * whitening to tackle numerical issues | |||
| */ | |||
| float64_t m_eigenvalue_zero_tolerance; | |||
|
|
|||
| private: | |||
| void init_with_evd(const SGMatrix<float64_t>& feature_matrix, int32_t max_dim_allowed); | |||
There was a problem hiding this comment.
These needs minimal doxygen documentation saying that they alter the state (transformation matrix).
Also potentially could say that EVD is based on an eigendocoposition of the covariance matrix, while SVD does an SVD on the data matrix.
fixed style (opening braces on new line)
|
One final request: Could you squash the commits? (or using git amend and force push to your fork) |
|
@karlnapf you can squash commits with a click now. "Squash and merge" |
* Moving EVD and SVD heavy lifiting into separate methods
Small refactoring PR to get to know the project and its processes