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

feat(symmetric): support SPD GPU #112

Merged
merged 6 commits into from
Mar 12, 2024

Conversation

JieRen98
Copy link
Contributor

@JieRen98 JieRen98 commented Dec 26, 2023

This PR

  • Symmetric struct
  • Symmetric GPU solver
  • Reformat
  • Symmetric matching support
  • Symmetric equilibration support

Future

  • Symmetric MAGMA support
  • Symmetric distributed support
  • Symmetric Low Rank support

@JieRen98 JieRen98 mentioned this pull request Dec 26, 2023
src/sparse/fronts/CMakeLists.txt Outdated Show resolved Hide resolved
src/sparse/fronts/FrontFactory.cpp Outdated Show resolved Hide resolved
src/StrumpackOptions.hpp Show resolved Hide resolved
@GitHubbeer
Copy link
Contributor

I observed that the matching process in STRUMPACK relies on the mc64ad algorithm (STRUMPACK ./src/sparse/MC64ad.cpp), which unfortunately does not support symmetric matrices. I found an alternative approach for Cholesky decomposition of symmetric matrices by incorporating the mc64_matching from HSL_MC64 ("matrix type indicates if the matrix is symmetric or not". section 2.5 in https://www.hsl.rl.ac.uk/specs/hsl_mc64_C.pdf). So I think we should implement our own symmetric version or use HSL_MC64 directly. I suggest that we can make it a separate pull request.

@JieRen98 JieRen98 changed the title feat(symmetric): support symmetric GPU feat(symmetric): support SPD GPU Feb 3, 2024
@GitHubbeer
Copy link
Contributor

@pghysels Could you please check this PR?

@pghysels
Copy link
Owner

pghysels commented Feb 3, 2024

Can you add a few tests?
You can run testSymmetricPositiveDefinite and testMixedPrecisionSymmetricPositiveDefinite.cpp in test/CMakeLists.txt.
You can download a small SPD matrix in download_mtx.sh.

Maybe you can rename files like testSymmetricPositiveDefinite to testSPD.

@pghysels
Copy link
Owner

pghysels commented Feb 3, 2024

Maybe we should add a routine like set_matrix that takes a symmetric matrix, i.e., only taking the lower/upper triangle.

The current CSRMatrix class (and CompressedSparseMatrix) have a symm_sparse argument. But this flag was only meant to denote whether the sparsity pattern is symmetric. It is not about the value symmetry or positive definiteness. So I feel like we should not confuse these things.

@JieRen98
Copy link
Contributor Author

JieRen98 commented Feb 3, 2024

How did you format codes? @pghysels

@pghysels
Copy link
Owner

pghysels commented Feb 3, 2024

How did you format codes? @pghysels

I don't use any strict formatting guidelines.
We should probably setup auto formatting using clang-format or something like that.

@JieRen98
Copy link
Contributor Author

JieRen98 commented Feb 3, 2024

How did you format codes? @pghysels

I don't use any strict formatting guidelines. We should probably setup auto formatting using clang-format or something like that.

Sounds great! I opened a new issue (#114) for this. I think we can do this in the next PR for clarity because I usually follow the philosophy that one PR for one feature.

@GitHubbeer
Copy link
Contributor

"test_SPD_mixedPrecison" and "test_SPD_seq" in test/CMakeList and routine "set_lower_triangle_matrix" for taking a symmetric matrix are ready. Please review. I also changed symm_sparse() to is_symmetric(opts_) to check whether the matrix is symmetric.

@pghysels
Copy link
Owner

This looks good. I will merge and then clean up a bit myself.

There might be licensing issue with the newer MC64 version.

@pghysels pghysels marked this pull request as ready for review March 12, 2024 20:55
@pghysels pghysels merged commit 08168ef into pghysels:master Mar 12, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants