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

Feature/spectra correlations #306

Merged
merged 13 commits into from
Jun 17, 2024

Conversation

atrayees
Copy link
Contributor

I made the following changes-

  1. wrote a new function to check if a matrix is indeed a correlation matrix
  2. ran the sampler for correlation matrices of greater sizes
  3. changed the ncv value in volesti/matrix_operations/EigenvaluesProblems.h from 15 to a more useful value
  4. added the spectra routine implementation in the minPosLinearEigenvalue_EigenSymSolver function

include the spectra implementation in the NT minPosLinearEigenvalue_EigenSymSolver
this function checks if a matrix is indeed a correlation matrix
Copy link
Contributor

@vfisikop vfisikop left a 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.

examples/correlation_matrices/sampler.cpp Outdated Show resolved Hide resolved
examples/correlation_matrices/sampler.cpp Outdated Show resolved Hide resolved
examples/correlation_matrices/sampler.cpp Outdated Show resolved Hide resolved
examples/correlation_matrices/sampler.cpp Outdated Show resolved Hide resolved
@@ -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);
Copy link
Contributor

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 ?

Copy link
Member

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).

Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Member

Choose a reason for hiding this comment

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

Please fix indentation here

Copy link
Contributor Author

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++;
Copy link
Member

Choose a reason for hiding this comment

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

Please fix indentation here

Copy link
Contributor Author

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
Copy link
Member

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i updated the comment.

Copy link
Contributor

@vfisikop vfisikop left a 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!

@vfisikop vfisikop merged commit e484865 into GeomScale:develop Jun 17, 2024
25 of 27 checks passed
TolisChal pushed a commit that referenced this pull request Jul 7, 2024
TolisChal pushed a commit that referenced this pull request Jul 7, 2024
TolisChal pushed a commit to TolisChal/volume_approximation that referenced this pull request Jul 7, 2024
TolisChal pushed a commit to TolisChal/volume_approximation that referenced this pull request Jul 7, 2024
TolisChal pushed a commit to TolisChal/volume_approximation that referenced this pull request Jul 8, 2024
TolisChal pushed a commit to TolisChal/volume_approximation that referenced this pull request Jul 8, 2024
TolisChal pushed a commit to TolisChal/volume_approximation that referenced this pull request Jul 8, 2024
TolisChal pushed a commit that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants