-
Notifications
You must be signed in to change notification settings - Fork 116
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
Feature/spectra correlations #306
Feature/spectra correlations #306
Conversation
include the spectra implementation in the NT minPosLinearEigenvalue_EigenSymSolver
this function checks if a matrix is indeed a correlation matrix
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 for this PR! I have a few comments.
@@ -168,8 +168,9 @@ class EigenvaluesProblems<NT, Eigen::Matrix<NT,Eigen::Dynamic,Eigen::Dynamic>, E | |||
Spectra::DenseCholesky<NT> Bop(-A); | |||
|
|||
// Construct generalized eigen solver object, requesting the largest three generalized eigenvalues | |||
int ncv = std::min(std::max(10, matrixDim/20), matrixDim); |
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.
Could you please comment on where those numbers come from? 10, 20, 15
?
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.
This is an empirical value I gave to @atrayees. We left as a task after the R interface to tune this implementation (not only ncv value).
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 @TolisChal for the explanations. @atrayees could you please add a comment with this explanation and mention that as a TODO?
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.
yes, done
} | ||
} | ||
|
||
//check if the matrix is positive definite |
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.
Why not using the function isPositiveSemidefinite
. Note: that function is not using tolerance.
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.
I have updated the function and made use of the isPositiveSemidefinite function
|
||
if(eigen_solver.info() != Eigen::Success) | ||
{ | ||
return false; |
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.
Please fix indentation here
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.
done
int valid_points = 0; | ||
for(const auto& points : randPoints){ | ||
if(is_correlation_matrix(points.mat)){ | ||
valid_points++; |
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.
Please fix indentation here
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.
done
@@ -168,8 +168,11 @@ class EigenvaluesProblems<NT, Eigen::Matrix<NT,Eigen::Dynamic,Eigen::Dynamic>, E | |||
Spectra::DenseCholesky<NT> Bop(-A); | |||
|
|||
// Construct generalized eigen solver object, requesting the largest three generalized eigenvalues |
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.
"... computing the minimum positive eigenvalue by computing the largest eigenvalue of the inverse Generalized Eigenvalue Problem."
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.
i updated the comment.
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 for the changes!
I made the following changes-
volesti/matrix_operations/EigenvaluesProblems.h
from 15 to a more useful valueminPosLinearEigenvalue_EigenSymSolver
function