Andrew/sccg and matrix class by andrewxu319 · Pull Request #421 · ORNL/ReSolve

andrewxu319 · 2026-05-15T09:07:37Z

Description

Implemented Schur complement conjugate gradient and Matrix parent class.

Proposed changes

Added a CPU implementation of Schur complement conjugate gradient similar to the HyKKT implementation, along with a basic test that will need to be completed later. The current test is a modified copy of the tests in the HyKKT implementation, which uses a set of test matrix files.

Created a Matrix class that is a parent of Sparse and, in the future, other matrix representations. The Matrix class is designed to act as an interface for matrices generally and has wrappers for matrix operations implemented in MatrixHandler.

@shakedregev

Checklist

Further comments

…a simple test.

pelesh

Very nice work, thank you. Please remove matrix::Matrix class so we can merge this.

pelesh · 2026-05-15T12:08:01Z

Some tests failing, need to fix that, as well. Setting this to draft PR until blocker issues are resolved.

…ale tests to fail. Made "createIncrementingVector()" return a pointer to the new vector instead of a copy (which causes the data to be freed prematurely).

shakedregev

Agree with Slaven's comments. Make sure all previous tests pass.
Slaven and I discussed yesterday and decided we don't need a matrix parent class. Sorry for the confusion.
Please pull my change before continuing.

No commented out code.

…arse" This reverts commit 00f6992. Kept changes to "io.cpp", which fixes the an issue with out-of-bounds iterators.

…e into andrew/sccg-and-matrix-class

andrewxu319 · 2026-05-15T15:53:30Z

Done--removed the Matrix class, and it's passing all of the tests.

shakedregev · 2026-05-15T16:27:30Z

Done--removed the Matrix class, and it's passing all of the tests.

Good work on this. I made a small correction. I tested the codes on CUDA and HIP and the good news is that at least the old tests are still passsing. Before we merge this, can you do the HIP/CUDA implementation as well? It's not working currently.

* use caller-provided backend-specific handlers in SCCG * load SCCG data on host before device sync * refresh CUDA SpMV cache when matrix changes * fix HyKKT SCCG HIP target setup * document SCCG backend setup and CUDA SpMV cache handling * mirror SpMV cache reset for HIP * Shaked/random error fix (#424) Fixed stochastic failures due to a faulty random number generator and asynchronous operations. Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> --------- Co-authored-by: tamar-dewilde <tamar-dewilde@users.noreply.github.com> Co-authored-by: Shaked Regev <35384901+shakedregev@users.noreply.github.com> Co-authored-by: shakedregev <shakedregev@users.noreply.github.com>

shakedregev

Good work on this, I think it's ready for merging.

The review is stale. See current changes.

pelesh

Good work, some minor documentation (code commenting) and style issues need to be resolved before merging.

pelesh · 2026-06-01T21:11:03Z

+
+      if (it == tmp.end())
+      {
+        return 0;
+      }
+
+      while (it != std::prev(tmp.end()))


Put comments to explain what is going on here. This code looks a little cryptic.

pelesh · 2026-06-01T21:11:59Z

-    cusparseCreateDnVec(&vecx, A->getNumRows(), vec_x->getData(memory::DEVICE), CUDA_R_64F);
+    cusparseCreateDnVec(&vecx, A->getNumColumns(), vec_x->getData(memory::DEVICE), CUDA_R_64F);



This is pretty big change. Needs explanation and comments in the code.

pelesh · 2026-06-01T21:12:46Z

+    // Rebuild cached SpMV setup if the matrix object or dimensions changed.
+    bool matrix_changed = (matrix_for_matvec_ != A) || (matvec_num_rows_ != A->getNumRows()) || (matvec_num_cols_ != A->getNumColumns()) || (matvec_nnz_ != A->getNnz());
+    if (matrix_changed || values_changed_)
+    {


Break this line per style guidelines.

pelesh · 2026-06-01T21:13:48Z

+    matrix::Sparse* matrix_for_matvec_{nullptr}; ///< Matrix used for cached SpMV setup.
+    index_type      matvec_num_rows_{0};         ///< Number of rows in cached SpMV matrix.
+    index_type      matvec_num_cols_{0};         ///< Number of columns in cached SpMV matrix.
+    index_type      matvec_nnz_{0};              ///< Number of nonzeros in cached SpMV matrix.


Why is matrix cached to begin with?

pelesh · 2026-06-01T21:14:45Z

+    // Rebuild cached SpMV setup if the matrix object or dimensions changed.
+    bool matrix_changed = (matrix_for_matvec_ != A) || (matvec_num_rows_ != A->getNumRows()) || (matvec_num_cols_ != A->getNumColumns()) || (matvec_nnz_ != A->getNnz());
+    if (matrix_changed || values_changed_)
+    {
+      workspace_->resetMatvecSetup();
+    }


Break the long line.

pelesh · 2026-06-01T21:19:49Z

+      HykktSchurComplementConjugateGradientTests(memory::MemorySpace memspace, MatrixHandler& matrix_handler, VectorHandler& vector_handler, std::mt19937& generator)
+        : memspace_(memspace), matrix_handler_(matrix_handler), vector_handler_(vector_handler), generator_(generator)
+      {


Break long lines per style guidelines.

andrewxu319 and others added 3 commits May 15, 2026 14:49

Create Matrix class, a parent class for matrices including Sparse

00f6992

Added CPU implementation for Schur Complement Conjugate Gradient and …

144d1ff

…a simple test.

Apply pre-commmit fixes

4ccb8af

pelesh previously requested changes May 15, 2026

View reviewed changes

pelesh assigned andrewxu319 May 15, 2026

pelesh marked this pull request as draft May 15, 2026 12:00

pelesh added enhancement New feature or request iterative solver labels May 15, 2026

andrewxu319 added 3 commits May 15, 2026 21:20

Fixed bug in MatrixHandlerTests.hpp that caused leftScale and rightSc…

e5d2d18

…ale tests to fail. Made "createIncrementingVector()" return a pointer to the new vector instead of a copy (which causes the data to be freed prematurely).

Add to CHANGELOG.md. Removed some unnecessary comments.

0214462

Bug fixes (missing <cmath>, type mismatch of variable "i").

7f91849

shakedregev reviewed May 15, 2026

View reviewed changes

Comment thread tests/unit/hykkt/HykktSCCGTests.hpp Outdated

shakedregev and others added 9 commits May 15, 2026 09:58

Update tests/unit/hykkt/HykktSCCGTests.hpp

1518323

No commented out code.

Apply pre-commmit fixes

ec40032

Revert "Create Matrix class, a parent class for matrices including Sp…

21559d4

…arse" This reverts commit 00f6992. Kept changes to "io.cpp", which fixes the an issue with out-of-bounds iterators.

Merge branch 'andrew/sccg-and-matrix-class' of github.com:ORNL/ReSolv…

90f1fcb

…e into andrew/sccg-and-matrix-class

Apply pre-commmit fixes

60cb831

Formatting fixes.

e031386

Merge branch 'andrew/sccg-and-matrix-class' of github.com:ORNL/ReSolv…

91bb428

…e into andrew/sccg-and-matrix-class

Removed "Matrix.hpp" from resolve/matrix/CMakeLists.txt

7b2018c

Apply pre-commmit fixes

d753b6e

shakedregev marked this pull request as ready for review May 15, 2026 16:05

shakedregev reviewed May 15, 2026

View reviewed changes

Comment thread resolve/hykkt/sccg/CMakeLists.txt

Apply suggestion from @shakedregev

fe13410

shakedregev requested a review from pelesh May 15, 2026 16:14

fixed not building test, test still fails on HIP

6f52fd0

Merge branch 'develop' into andrew/sccg-and-matrix-class

c7075e2

shakedregev approved these changes May 29, 2026

View reviewed changes

pelesh requested changes Jun 1, 2026

View reviewed changes

		cusparseCreateDnVec(&vecx, A->getNumRows(), vec_x->getData(memory::DEVICE), CUDA_R_64F);
		cusparseCreateDnVec(&vecx, A->getNumColumns(), vec_x->getData(memory::DEVICE), CUDA_R_64F);

Conversation

andrewxu319 commented May 15, 2026 • edited by shakedregev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed changes

Checklist

Further comments

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

pelesh commented May 15, 2026

Uh oh!

shakedregev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewxu319 commented May 15, 2026

Uh oh!

Uh oh!

shakedregev commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shakedregev left a comment

Choose a reason for hiding this comment

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

pelesh Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

pelesh Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

pelesh Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

pelesh Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

pelesh Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

pelesh Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrewxu319 commented May 15, 2026 •

edited by shakedregev

Loading

shakedregev left a comment •

edited

Loading

shakedregev commented May 15, 2026 •

edited

Loading