Skip to content

Add ReSolve CSR Matrix#292

Merged
pelesh merged 22 commits intodevelopfrom
add-resolve-csr
Nov 3, 2025
Merged

Add ReSolve CSR Matrix#292
pelesh merged 22 commits intodevelopfrom
add-resolve-csr

Conversation

@alexander-novo
Copy link
Collaborator

@alexander-novo alexander-novo commented Oct 28, 2025

Description

Adds the ReSolve Csr matrix class to GridKit, with minimal changes:

  • Namespaces have been changed to be consistent with GridKit
  • Csr class has been changed to CsrMatrix
  • ReSolve logging code has been removed, and replaced with std::cerr
  • GPU structure has been commented out to be ported later

Tracking issue: #261

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.
  • I have updated CHANGELOG.md to reflect the changes in this PR. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further comments

Follow-up PR will merge Sparse into CsrMatrix.

Something I'd like to fix is the potential to double free if the matrix is copied. This should probably be fixed by removing the ability to copy the matrix.

@alexander-novo alexander-novo self-assigned this Oct 28, 2025
@alexander-novo alexander-novo added the enhancement New feature or request label Oct 28, 2025
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Great job! A couple of items to address before this can be merged:

  • Remove Sparse base class and include its content in CsrMatrix.
  • Remove LinAlgWorkspaceCpu class. It is not needed here.

There are a few other mainly nitpicking issues I suggest we address.

@pelesh
Copy link
Collaborator

pelesh commented Oct 28, 2025

Follow-up PR will merge Sparse into CsrMatrix.

I suggest you merge Sparse into CsrMatrix in this PR. It is a minor modification.

@pelesh
Copy link
Collaborator

pelesh commented Oct 28, 2025

Something I'd like to fix is the potential to double free if the matrix is copied. This should probably be fixed by removing the ability to copy the matrix.

Perhaps just implement the assignment operator and make it private method for now? I would leave this for a different PR though.

@alexander-novo
Copy link
Collaborator Author

@pelesh I would prefer deleting any copy operators. Implicitly copying something like a CSR matrix is a fast track to difficult to find performance decreases for no good reason.

Copy link
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

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

Documentation needs improvement. Otherwise, just the things we discussed.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Almost there. The CsrMatrix needs to be implemented as a class template. There are a few other nitpicking issues I suggest we address.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Looks good. I suggest you fix a few warnings popping up and rebase. We can merge then.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

All review comments addressed.

@pelesh pelesh merged commit d21447d into develop Nov 3, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants