Skip to content

Conversation

@eulovi
Copy link
Contributor

@eulovi eulovi commented Dec 31, 2025

Description and Context

In this PR, the add_other() methods are deleted from SparseOperator and its sub-classes. In the current implementation, these methods where used to call the overloaded add() functions from the SparseOperator for the sub-classes SparseMatrix and BlockSparseMatrixBase, which results in confusing code. I think this is done to be sure that only a SparseMatrix is added to a SparseMatrix instance and a BlockSparseMatrixBase added to a BlockSparseMatrixBase instance.

In this PR, a dynamic_cast is added to be sure that the correct matrix types are added.

Related Issues and Pull Requests

Related to #136

@eulovi eulovi requested a review from maxfirmbach December 31, 2025 14:44
@eulovi eulovi self-assigned this Dec 31, 2025
@eulovi eulovi added taskforce: tpetra Issues related to the migration from Epetra to Tpetra type: clean code Clean coding and good programming practices labels Dec 31, 2025
Copy link
Contributor

@maxfirmbach maxfirmbach 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 for me! Thanks for removing the add_other() construct.

I just a have follow up question. How do we want to handle the following?

void Core::LinAlg::BlockSparseMatrixBase::add(const Core::LinAlg::BlockSparseMatrixBase& A,
const bool transposeA, const double scalarA, const double scalarB)`

To me it seems like this is just a copy of the other add() method using Core::LinAlg::BlockSparseMatrixBase instead of Core::LinAlg::SparseOperator. Could this be further simplified?

The final remark for a further PR would be, if we want to keep the add() method in general or try to put it into a free function, or modify the matrix_add() free function to be able to handle all cases.

auto* blocksparse_matrix = dynamic_cast<const Core::LinAlg::BlockSparseMatrixBase*>(&A);
FOUR_C_ASSERT(blocksparse_matrix != nullptr,
"Matrix A cannot be added to this block sparse matrix as it is not a block sparse matrix!");

Copy link
Member

Choose a reason for hiding this comment

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

We should check whether both matrices have the same size:

Suggested change
FOUR_C_ASSERT(blocksparse_matrix.rows() == rows(), "The number of rows of the block matrix does not match: {} vs {}.", blocksparse_matrix.rows(), rows());
FOUR_C_ASSERT(blocksparse_matrix.cols() == cols(), "The number of colums of the block matrix does not match: {} vs {}.", blocksparse_matrix.cols(), cols());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

taskforce: tpetra Issues related to the migration from Epetra to Tpetra type: clean code Clean coding and good programming practices

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants