Cleanup: remove unneeded argument for `Preconditioner::Update` by RemiLehe · Pull Request #7234 · BLAST-WarpX/warpx

RemiLehe · 2026-09-02T16:56:43Z

I found the interface of Preconditioner::Update confusing, because we pass a_X (a WarpXSolverVec) to it, but in practice the information from a_X is never really used.

(Instead Update uses other objects, stored internally in the Preconditioner object, to update the equation it needs to solve.)

This PR aims to make the interface less confusing by simply not passing a_X to Update.

None of the preconditioners re-linearize about the vector passed to `Update()`, so the argument only suggests a dependence that does not exist. Of the three implementations, `CurlCurlMLMGPC` ignored it outright, while `JacobiPC` and `MatrixPC` used it purely for layout and index metadata, never for the field values: - `JacobiPC` read `boxArray()`/`DistributionMap()` to size the scratch MultiFabs it allocates lazily. These are now cached in `Define()`, which already receives the same vector. The allocation stays lazy: its guard is on the mass-matrix coefficients being set, not on the vector. - `MatrixPC::Assemble()` read `getDOFsObject()`. The solver vector class owns a single DOF object that lives until AMReX finalization, so this is now cached in `Define()` as a non-owning pointer as well. The state a preconditioner actually depends on reaches it through the `Ops` object and through `CurTime()`/`CurTimeStep()`, so dropping the argument makes the real data flow explicit. Should a preconditioner ever need the current iterate, it can be added back at that point. `LinearFunction::updatePreCondMat()` exists only to forward to `Update()`, so it loses its argument too, along with its call sites in `NewtonSolver`, `WarpX_PETSc` and `DarwinLinearFieldOperator`. Also drop a stale `ignore_unused(a_U)` in `MatrixPC::Define()`, whose `// a_U is not needed` comment contradicted the three uses of `a_U` immediately below it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

for more information, see https://pre-commit.ci

The `decltype(std::declval<const T&>().getDOFsObject().get())` alias was over-engineered: `MatrixPC` already hard-codes `FieldType::Efield_fp`, so it is not generic, and the alias was hard to read for no benefit. `Assemble()` only ever needed the DOF index maps, so cache those directly. Their type is expressible in plain AMReX terms, which removes the alias without naming `WarpXSolverDOF` or adding a NonlinearSolvers -> ImplicitSolvers include. Also drop the two DOF-count asserts. `WarpXSolverVec` owns a single static DOF object shared by every solver vector, so these compared `m_ndofs_*` against the very storage they had been copied from in `Define()` and could not fail. The separate null check is gone too: the `nDOF_local()`/`nDOF_global()` queries just above already assert that the DOF object exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`WarpXSolverVec` owns a single static DOF object shared by every solver vector, so `Assemble()` never needed an instance to reach it, and nothing needed caching in the first place. Make `getDOFsObject()` static -- it already returned static state, so the `const` member form was misleading -- and read the index maps through it directly. This removes the cached member, the `decltype(std::declval<...>())` alias that spelled its type, the caching block in `Define()`, its null-check assert, and the `<utility>` include. `MatrixPC.H` now differs from before this PR only in the `Update()`/`Assemble()` signatures and this one line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>

RemiLehe · 2026-09-02T22:32:47Z

-    // Get DOF object from a_U
-    const auto& dofs_obj = a_U.getDOFsObject();
-    const auto& dofs_mfarrvec = dofs_obj->m_array;
-    AMREX_ALWAYS_ASSERT(m_ndofs_l == dofs_obj->m_nDoFs_l);


This PR removes the AMREX_ALWAYS_ASSERT. On the other hand, I am not sure that this ASSERT was needed. Are there really configurations where m_ndofs_l could become different from dofs_obj->m_nDoFs_l?

@debog I'll let Debo answer that question. It may not be needed, but I"m not sure.

RemiLehe · 2026-09-02T22:39:53Z


    // return DOFs object pointer
-    [[nodiscard]] inline const auto& getDOFsObject () const { return m_dofs; }
+    [[nodiscard]] inline static const auto& getDOFsObject () { return m_dofs; }


This function can be made static here (i.e., it can be called with WarpXSolverVec::getDOFsObject instead of requiring an instance of WarpXSolverVec) since m_dofs itself is declared as a static member (i.e. one m_dofs object per class, not per instance)

JustinRayAngus

Thanks!

Adapt the Darwin MLMG preconditioner to the new Preconditioner::Update() interface from BLAST-WarpX#7234 (the WarpXSolverVec argument was removed).

RemiLehe changed the title ~~[WIP] Remove unused vector argument from Preconditioner::Update~~ [WIP] Remove vector argument from Preconditioner::Update Sep 2, 2026

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/NonlinearSolvers/Preconditioner.H Outdated

RemiLehe force-pushed the simplify_pc_update_interface branch from edc3a49 to 83b6cda Compare September 2, 2026 18:15

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/NonlinearSolvers/JacobiPC.H

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/NonlinearSolvers/JacobiPC.H Outdated

RemiLehe and others added 3 commits September 2, 2026 14:02

Apply suggestion from @RemiLehe

d80b0f8

[pre-commit.ci] auto fixes from pre-commit.com hooks

23092a5

for more information, see https://pre-commit.ci

RemiLehe changed the title ~~[WIP] Remove vector argument from Preconditioner::Update~~ Remove vector argument from Preconditioner::Update Sep 2, 2026

RemiLehe changed the title ~~Remove vector argument from Preconditioner::Update~~ [WIP] Remove vector argument from Preconditioner::Update Sep 2, 2026

RemiLehe and others added 2 commits September 2, 2026 15:21

Apply suggestion from @RemiLehe

0a7d1f9

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/FieldSolver/ImplicitSolvers/WarpXSolverVec.H Outdated

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/NonlinearSolvers/MatrixPC.H Outdated

Apply batched suggestions from code review

afed726

Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/FieldSolver/ImplicitSolvers/WarpXSolverVec.H Outdated

Apply suggestion from @RemiLehe

cbaedcb

RemiLehe commented Sep 2, 2026

View reviewed changes

Comment thread Source/FieldSolver/ImplicitSolvers/WarpXSolverVec.H Outdated

Apply suggestion from @RemiLehe

1fd23b3

RemiLehe commented Sep 2, 2026

View reviewed changes

RemiLehe changed the title ~~[WIP] Remove vector argument from Preconditioner::Update~~ Remove vector argument from Preconditioner::Update Sep 2, 2026

RemiLehe changed the title ~~Remove vector argument from Preconditioner::Update~~ Cleanup: Remove vector argument from Preconditioner::Update Sep 2, 2026

RemiLehe changed the title ~~Cleanup: Remove vector argument from Preconditioner::Update~~ Cleanup: remove unneeded argument for Preconditioner::Update Sep 2, 2026

RemiLehe requested a review from JustinRayAngus September 2, 2026 22:40

RemiLehe assigned JustinRayAngus Sep 2, 2026

JustinRayAngus approved these changes Sep 2, 2026

View reviewed changes

RemiLehe enabled auto-merge (squash) September 2, 2026 23:43

RemiLehe added the cleaning Clean code, improve readability label Sep 2, 2026

RemiLehe merged commit 47ef223 into BLAST-WarpX:development Sep 3, 2026
51 checks passed

RemiLehe added a commit to roelof-groenewald/WarpX that referenced this pull request Sep 3, 2026

Merge branch 'development' into pr7180

98f1a68

Adapt the Darwin MLMG preconditioner to the new Preconditioner::Update() interface from BLAST-WarpX#7234 (the WarpXSolverVec argument was removed).

Navigation Menu

Uh oh!

Cleanup: remove unneeded argument for `Preconditioner::Update` - #7234

Cleanup: remove unneeded argument for `Preconditioner::Update`#7234
RemiLehe merged 9 commits into
BLAST-WarpX:developmentfrom
RemiLehe:simplify_pc_update_interface

RemiLehe commented Sep 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RemiLehe Sep 2, 2026 •

edited

Loading

Uh oh!

JustinRayAngus Sep 2, 2026

Uh oh!

Uh oh!

Uh oh!

RemiLehe Sep 2, 2026

Uh oh!

JustinRayAngus left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

RemiLehe commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RemiLehe Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinRayAngus Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RemiLehe Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

JustinRayAngus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RemiLehe commented Sep 2, 2026 •

edited

Loading

RemiLehe Sep 2, 2026 •

edited

Loading