Skip to content

chore: Refactor linear solver to have pre conditioner baked in the same class - #501

Closed
anushkasinghh wants to merge 18 commits into
pyccel:develfrom
anushkasinghh:anushka/preconditionerRefactoring
Closed

chore: Refactor linear solver to have pre conditioner baked in the same class#501
anushkasinghh wants to merge 18 commits into
pyccel:develfrom
anushkasinghh:anushka/preconditionerRefactoring

Conversation

@anushkasinghh

@anushkasinghh anushkasinghh commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

Refactor linear solver to have pre conditioner baked in the same class. Pre conditioner is now a parameter rather than a different class altogether.

The following is done in the pull request more granularly :

  1. Merge ConjugateGradient and PConjugatreGradient
  2. Merge BiConjugateGradientStabilized and PBiConjugateGradientStabilized

Will solve #469 when merged

@anushkasinghh
anushkasinghh force-pushed the anushka/preconditionerRefactoring branch from 8e08a8c to 88d02e4 Compare June 4, 2025 10:44
@anushkasinghh
anushkasinghh force-pushed the anushka/preconditionerRefactoring branch from 508bca3 to ee6ceab Compare June 13, 2025 09:07
Anushka Singh and others added 9 commits June 17, 2025 16:51
… on presence of preconditioner.

- Clarified separation of `solve_with_pc` and `solve_without_pc`, improving solver flexibility and consistency.
- Updated test_solver_tridiagonal to handle uninitialized 'pc' variable:
  - Avoided UnboundLocalError by checking for 'pc' using 'locals()' before referencing.
- Updated test_x0_update
Previously, CG and BiCG defined the `.solve` method dynamically inside `.dot()`, based on whether a preconditioner was provided. This caused errors when `.solve()` was called on transposed solver objects, since `.dot()` had not been invoked yet and `.solve` was not initialized.

This commit moves the `.solve` assignment logic into the constructor (`__init__`), ensuring `.solve` is always available and correctly bound, even for transposed instances. This makes CG/BiCG consistent with other solver classes where `.solve()` is always defined.
@anushkasinghh

anushkasinghh commented Jul 4, 2025

Copy link
Copy Markdown
Contributor Author

While debugging the bicgstab solver (using the old implementation of the algorithm), I found that it consistently fails for complex systems when using a diagonal preconditioner (pc = A.diagonal(inverse=True)). However, all tests pass when I set pc = None. This issue appears to affect only bicgstab with complex dtype — all other solver/dtype combinations behave as expected.

It looks like the preconditioner is destabilizing the method for complex matrices. Would you recommend skipping the preconditioner in this specific case @yguclu ?


========================================================================== short test summary info ===========================================================================
FAILED psydac/linalg/tests/test_solvers.py::test_solver_tridiagonal[bicgstab-complex-2-5] - assert np.float64(1.8472203713614275e+24) < 1e-08
FAILED psydac/linalg/tests/test_solvers.py::test_solver_tridiagonal[bicgstab-complex-2-10] - assert np.float64(786.0899812535919) < 1e-08
FAILED psydac/linalg/tests/test_solvers.py::test_solver_tridiagonal[bicgstab-complex-2-13] - assert np.float64(8.004021243308681) < 1e-08
FAILED psydac/linalg/tests/test_solvers.py::test_solver_tridiagonal[bicgstab-complex-3-5] - assert np.float64(49927742822871.3) < 1e-08
FAILED psydac/linalg/tests/test_solvers.py::test_solver_tridiagonal[bicgstab-complex-3-10] - assert np.float64(0.0005220081883679691) < 1e-08
FAILED psydac/linalg/tests/test_solvers.py::test_solver_tridiagonal[bicgstab-complex-3-13] - assert False
================================================================== 6 failed, 66 passed, 5 warnings in 4.79s 

Anushka Singh added 3 commits July 4, 2025 15:14
…s to divergence of solution for complex systems in test_solver_tridiagonal
…Gleichungssysteme".

Set pc = None for bicgstab with pc, as solution doesnt converge otherwise.
@anushkasinghh

Copy link
Copy Markdown
Contributor Author

changes migrated to pr512

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.

2 participants