Skip to content

Conversation

@gojakuch
Copy link
Collaborator

@gojakuch gojakuch commented Oct 22, 2025

This PR is basically #1932 but without the sorting check. I've also added a test for a solver with the Schwarz preconditioner with L1 smoothing
Fixes: #1920

@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. type:preconditioner This is related to the preconditioners labels Oct 22, 2025
@gojakuch gojakuch force-pushed the fix/sorting-diag branch 2 times, most recently from af8c930 to 9cc6d8f Compare October 28, 2025 20:10
@gojakuch
Copy link
Collaborator Author

gojakuch commented Oct 29, 2025

I lowered the test tolerance for gko::half a bit

@gojakuch gojakuch force-pushed the fix/sorting-diag branch 2 times, most recently from 191146a to f32eb8c Compare October 30, 2025 11:02
@gojakuch gojakuch marked this pull request as ready for review October 30, 2025 11:02
Comment on lines 7 to 8
#include <cmath>
#include <memory>
Copy link
Member

Choose a reason for hiding this comment

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

Is it needed?

Comment on lines 234 to 235
l1_diag_csr->sort_by_column_index(); // spgeam requires sorting for
// some backends
Copy link
Member

Choose a reason for hiding this comment

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

Does it indeed solve the problems?
l1_diag should only contain the diagonal entry per row.
The column index should be perfectly sorted because of only one entry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, this totally makes sense... I'm not sure how I haven't noticed this, maybe I was just too overwhelmed trying to understand the preconditioner logic generally. I don't even know what the issue is about then, as I thought that it's described very clearly.

@ginkgo-project ginkgo-project deleted a comment from ginkgo-bot Nov 4, 2025
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

the new test seems to be wrong.
Could you also check whether it is failed before fix?
The dist_mtx should already store the matrix in order.
You need to create the distributed matrix by hand first.

Comment on lines +421 to +423
.with_generated_preconditioner(this->local_solver_factory->generate(
gko::copy_and_convert_to<local_matrix_type>(
this->exec, non_dist_diag_with_l1)))
Copy link
Member

Choose a reason for hiding this comment

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

It is not the same matrix.
Schwarz L1 smoother adds the row absolute sum from non-local to diagonal of local matrix but it still keeps the off-diagonal matrix of local matrix.

using cg = typename TestFixture::solver_type;
using prec = typename TestFixture::dist_prec_type;
using local_matrix_type = typename TestFixture::local_matrix_type;
constexpr double tolerance = 1e-20;
Copy link
Member

Choose a reason for hiding this comment

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

tolerance should adapt with valuetype. you can get it by r<T>::value

@MarcelKoch MarcelKoch added this to the Ginkgo 1.11 milestone Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:run-full-test mod:core This is related to the core module. type:preconditioner This is related to the preconditioners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Schwarz with L1 can fail if diagonal matrix is not sorted

5 participants