Skip to content

Add diag sorting to Schwarz precond w/ l1 smoother#1939

Open
gojakuch wants to merge 8 commits intodevelopfrom
fix/sorting-diag
Open

Add diag sorting to Schwarz precond w/ l1 smoother#1939
gojakuch wants to merge 8 commits intodevelopfrom
fix/sorting-diag

Conversation

@gojakuch
Copy link
Collaborator

@gojakuch gojakuch commented Oct 22, 2025 •
edited
Loading

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 •
edited
Loading

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
yhmtsai
yhmtsai requested changes Oct 30, 2025
View reviewed changes
core/distributed/preconditioner/schwarz.cpp Outdated
Comment on lines 7 to 8
#include <cmath>
#include <memory>
Copy link
Member

@yhmtsai yhmtsai Oct 30, 2025

Choose a reason for hiding this comment

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

Is it needed?

core/distributed/preconditioner/schwarz.cpp Outdated
Comment on lines +234 to +235
l1_diag_csr->sort_by_column_index(); // spgeam requires sorting for
// some backends
Copy link
Member

@yhmtsai yhmtsai Oct 30, 2025

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

@gojakuch gojakuch Nov 1, 2025

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.

@yhmtsai yhmtsai mentioned this pull request Nov 3, 2025
Open
@ginkgo-project ginkgo-project deleted a comment from ginkgo-bot Nov 4, 2025
yhmtsai
yhmtsai requested changes Nov 10, 2025
View reviewed changes
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.

test/mpi/preconditioner/schwarz.cpp Outdated
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

@yhmtsai yhmtsai Nov 10, 2025

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.

@MarcelKoch MarcelKoch added this to the Ginkgo 1.11 milestone Nov 11, 2025
@gojakuch gojakuch force-pushed the fix/sorting-diag branch 2 times, most recently from f43a102 to 3276015 Compare December 17, 2025 18:42
@gojakuch gojakuch force-pushed the fix/sorting-diag branch 2 times, most recently from 6c57178 to f2e0827 Compare January 13, 2026 18:49
@gojakuch
Copy link
Collaborator Author

gojakuch commented Jan 13, 2026

@yhmtsai I made a completely new test and made sure that it wouldn't have been passed prior to these changes, so it seems to be correct. I also rebased this PR and cleaned up the commit history a bit. please tell me if this is ok, so that I add this PR to the changelog (I don't want to add a raw PR there).

@gojakuch gojakuch requested a review from yhmtsai January 13, 2026 18:58
@gojakuch gojakuch requested a review from pratikvn February 17, 2026 15:10
@gojakuch
Copy link
Collaborator Author

gojakuch commented Mar 5, 2026

@yhmtsai should I keep rebasing this PR? could you please take a look if what did here in the end matches what you had suggested?

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

Reviewers

@yhmtsai yhmtsai Awaiting requested review from yhmtsai

@pratikvn pratikvn Awaiting requested review from pratikvn

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

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

Milestone

Ginkgo 1.11

Development

Successfully merging this pull request may close these issues.

Schwarz with L1 can fail if diagonal matrix is not sorted

4 participants

@gojakuch @yhmtsai @MarcelKoch @ginkgo-bot