Skip to content

Fix FSILS RCS diagonal cancellation and general block scaling - #647

Open
zasexton wants to merge 4 commits into
SimVascular:mainfrom
zasexton:issue-646
Open

zasexton wants to merge 4 commits into
SimVascular:mainfrom
zasexton:issue-646

Conversation

@zasexton

@zasexton zasexton commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Current situation

Fixes #646.

The FSILS row and column scaling (RCS) preconditioner evaluates (a - 1.0) + 1.0 for unconstrained matrix diagonals while applying Dirichlet conditions. In double precision, this perturbs small coefficients and can erase them before scaling begins. The resulting operator no longer represents the assembled system, which can degrade convergence and solution accuracy across physics using this shared preconditioner.

For block sizes greater than four, column scaling also skips the last row of each block, and the row-maximum calculation omits the first entry. The latter can produce infinite scaling weights when a constrained diagonal is the only nonzero entry in its row.

Release Notes

  • Preserve unconstrained RCS diagonals and set only constrained diagonals to one, using the normalized boundary mask for every block size.
  • Correct the general block path to scale every block row and include every entry when computing row maxima.
  • Add RCS matrix regression coverage for small coefficients, boundary constraints, and block sizes one through five.

Documentation

The numerical cancellation guard is documented inline in Code/Source/linear_solver/precond.cpp. The regression test documents its sparse matrix stencil and scale-normalized error checks.

Testing

  • 327 unit tests passed, including 75 RCS matrix cases. These cover block sizes 1–5, coefficient scales 1, 1e-11, 1e-15, 1e-17, and 1e-20, and excluded, partially constrained, and fully constrained boundary faces.
  • The matrix tests verify finite scaling weights, the reconstructed operator and right-hand side, the recovered solution, and the residual against the original system. Error checks are normalized by coefficient scale so lost small entries cannot pass through an absolute tolerance.
  • Before their respective fixes, 48 small-coefficient cases reproduced diagonal corruption, including complete erasure, and 15 five-component cases reproduced the general block defects. All 75 cases pass with the fixes.
  • 15 existing physics regressions passed on 1, 3, and 4 MPI ranks: heat diffusion with CG, BICG, and GMRES; CEP spiral_BO_2d; and the Stokes P1P1/N004 manufactured solution.
  • Three additional Stokes comparisons passed, using temporary configurations that explicitly select RCS on 1, 3, and 4 MPI ranks.

Code of Conduct & Contributing Guidelines

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@zasexton

Copy link
Copy Markdown
Collaborator Author

adding @kko27 and @michelebucelli to review this PR since RCS preconditioning is occasionally used with CEP physics. Mostly want to know if we should add the unit test for regression assessments or instead remove that file and just have the bug fix only in this PR.

@kko27 kko27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seeing that this is the first unit test we have for the linear solver, maybe this could be a separate issue with a dedicated plan in creating a usable template.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do not think this needs to be in a separate PR. I do think that it would be nice to have a bit of documentation of what the test is doing so that it can be more easily adopted as a template/example to build other tests on.

I would recommend to expand the test's Doxygen documentation for this (Doxygen has a @test command for this purpose, I believe).

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.44%. Comparing base (c3f0bb8) to head (62b8cb3).

Files with missing lines Patch % Lines
...unitTests/linear_solver_tests/test_precond_rcs.cpp 98.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #647      +/-   ##
==========================================
+ Coverage   73.08%   73.44%   +0.35%     
==========================================
  Files         268      269       +1     
  Lines       40270    40338      +68     
  Branches     6738     6755      +17     
==========================================
+ Hits        29433    29625     +192     
+ Misses      10594    10469     -125     
- Partials      243      244       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michelebucelli michelebucelli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@zasexton I have no comments besides the one about detailing the test rationale a bit more.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FSILS RCS preconditioner alters small matrix diagonals and degrades convergence

3 participants