Add more tests for FCI operators #3196

ZedThree · 2025-11-04T11:50:14Z

Add MMS tests for all the main FCI derivative operators
A bunch of drive-by fixes for clang-tidy warnings

- Include all relevant headers, remove unused ones, add some forward declarations - Make data `private` instead of `protected` - Add getter instead of `const` member - Change member reference to pointer - Move ctor to .cxx file - Use `std::array` instead of C array - Bunch of other small clang-tidy fixes

The Div_par operators use parallel slices of B -- with 2D metrics, these are just the field itself, in 3D we need the actual slices. While `Coordinates::geometry` does communicate the fields, it puts off calculating the parallel slices because that needs the fully constructed `Coordinates`. Upcoming changes should fix this and remove the need to explicitly communicate `Coordinates` members.

* next: (78 commits) Apply clang-format changes Add `has_hypre` to python `boutconfig` Update examples for LaplaceXY factory Fix call to virtual function in ctor CI: Don't warn about including PETSc or MPI symbols directly Try to appease clang-tidy `misc-include-cleaner` Remove private variables from base class Fix typo in define Apply clang-format changes Add factory for LaplaceXY, LaplaceXY2, LaplaceXY2Hypre Enable non-const iteration over `Options` snes solver: Consider timestep change at output Bump DOI Add shim for ARKodeGetNumRhsEvals Suppress warning from `nodiscard` function Remove `boututils` from requirements; bump `boutdata` Fix deprecation warning Bump bundled fmt Fix some easy clang-tidy snes warnings Fix reorder warning from snes ...

github-actions

clang-tidy made some suggestions

tests/MMS/spatial/fci/fci_mms.cxx

tests/integrated/test-fci-mpi/fci_mpi.cxx

dschwoerer

Looks good!
Thanks @ZedThree

src/mesh/parallel/fci.cxx

dschwoerer · 2025-11-04T12:10:03Z

src/mesh/interpolation/hermite_spline_xz.cxx

+    // TODO(peter): Can we remove this if we apply (dirichlet?) BCs to
+    // the X derivatives? Note that we need at least _2_


I do not think that applying dirichlet BCs will be in general a good idea.
I think in general this is not really an issue - anything that takes the case below should be i_corn == xend and t_x = 0. Other cases should be boundaries, and should not take this path.

Ah, that's true about the boundaries. The pain here is for grids where there's only one point in either x or z, so this fudge ends up cancelling out and breaking things. That's probably not super common, so maybe we just need ASSERT4(mesh.Nx() >= 2) or something

Yes, I think that is only an issue for tests, that try to have a minimal grid. I do not think we should to much effort in for them ...

dschwoerer · 2025-11-04T12:10:39Z

src/mesh/interpolation/hermite_spline_xz.cxx

+  // TODO(peter): Should we apply dirichlet BCs to derivatives?
+


I do not think so. What would be the motivation? So we do not need two x-guard cells?

src/mesh/parallel/fci.cxx

dschwoerer · 2025-11-04T12:43:26Z

tests/MMS/spatial/fci/fci_mms.cxx

+    // Div_par operators require B parallel slices:
+    // Coordinates::geometry doesn't ensure this (yet)
+    auto& Bxy = mesh->getCoordinates()->Bxy;
+    mesh->communicate(Bxy);


In general you would need to apply parallel BCs, but I guess the tested grids do not have this?

Correct, no BCs set because all the input fields are created from expressions

I think B should be loaded also for the parallel slices, in which case we do not need to communicate at all ...

It doesn't seem to be the case -- the communicate was necessary here to get any parallel slices on Bxy

Yes, these changes are not in next, only in f3dwy (PR #3197 )

github-actions

clang-tidy made some suggestions

src/mesh/parallel/fci.cxx

Co-authored-by: David Bold <[email protected]>

ZedThree · 2025-11-06T17:43:37Z

A silly typo meant that the new FCI tests were always passing. Fixing that showed that the monotonichermitespline cases were actually failing. It's known that the interpolation is degraded around extrema, but the errors appear with a strange pattern:

Grad_par:

and they accumulate with higher derivatives:

Grad2_par2:

How widely used is monotonichermitespline in practice?

bendudson · 2025-11-06T18:06:36Z

Thanks @ZedThree ! monotonichermitespline is used in practice: It's been my favorite interpolator. It's useful because it should avoid overshoot oscillations in e.g. low-density regions.

dschwoerer · 2025-11-07T08:02:43Z

Note that this is not https://en.wikipedia.org/wiki/Monotone_cubic_interpolation - but hermitespline - with the value cropped to the bound of the 4 surrounding values.

dschwoerer · 2025-11-07T08:03:59Z

Ah, the test do not always pass - they still fail if order is negative 😇
I had failing tests when I merged this into f3dwy. I will check whether I also see this issue ...

ZedThree · 2025-11-07T11:23:19Z

Ok, so the errors are just coming from extrema in the input function. The question now is what is the appropriate fix:

don't test monotonichermitespline in this test, or possibly just skip it for the higher derivatives?
change the input function to have fewer/smaller extrema -- we could set yperiodic=False and have the function monotonic in y, although we'd then also introduce some dependence on the BCs, otherwise we need at least one extrema to make it periodic
change the implementation (or introduce a new interpolator) to control overshoots in a different manner, for example by damping the derivative parts to change the tension in the spline

For reference, here's the error scaling for the different operators for monotonichermitespline:

grad_par convergence order = 1.437273 (fit), 1.547833 (small spacing)
grad2_par2 convergence order = 0.682735 (fit), 0.723754 (small spacing)
div_par convergence order = 1.437273 (fit), 1.547833 (small spacing)
div_par_K_grad_par convergence order = 0.720145 (fit), 0.898745 (small spacing)
laplace_par convergence order = 0.682735 (fit), 0.723754 (small spacing)

Note that they're not even first order

This allows the user to control the clipping, allowing some overshoot Also restore the FCI MMS test with this interpolator

ZedThree · 2025-11-21T15:30:01Z

I've used @dschwoerer's suggestion and added rtol and atol parameters for monotonichermitespline, and it now successfully passes the MMS test. The default values give the old behaviour, but they are user configurable

github-actions

clang-tidy made some suggestions

github-actions · 2025-11-21T15:38:11Z

include/bout/interpolation_xz.hxx


 class XZInterpolationFactory
-    : public Factory<XZInterpolation, XZInterpolationFactory, Mesh*> {
+    : public Factory<XZInterpolation, XZInterpolationFactory, Mesh*, Options*> {


warning: no header providing "Factory" is directly included [misc-include-cleaner]

include/bout/interpolation_xz.hxx:27:

- #include "bout/mask.hxx" + #include "bout/generic_factory.hxx" + #include "bout/mask.hxx"

github-actions · 2025-11-21T15:38:11Z

include/bout/interpolation_xz.hxx


  ReturnType create(Options* options = nullptr, Mesh* mesh = nullptr) const {
-    return Factory::create(getType(options), mesh);
+    return Factory::create(getType(options), mesh, options);


warning: no header providing "Factory" is directly included [misc-include-cleaner]

return Factory::create(getType(options), mesh, options); ^

ZedThree and others added 25 commits October 6, 2025 14:22

CI: Don't run clang-{tidy,format} on RC branches

36f19fa

Fix reorder warning from snes

aaa4600

Fix some easy clang-tidy snes warnings

e48cc34

Bump bundled fmt

a855c06

Fix deprecation warning

e2cd97c

Remove boututils from requirements; bump boutdata

89b0855

Suppress warning from nodiscard function

1704a4a

Add shim for ARKodeGetNumRhsEvals

8c26fe3

tests: Ruff fixes for FCI runtest

ef744ea

tests: Expand FCI MMS test to Grad2_par2

d889ecf

tests: Generalise FCI MMS test to allow for more cases

122c39a

tests: Add test for FCI Div_par

1a0af58

tests: Make MMS script update input in-place

1e912bd

tests: Add test for FCI Div_par_K_Grad_par

560b005

tests: Add test for FCI Laplace_par

bdef58e

Reduce duplication in FCI mms script

969997c

Remove circular include

3468db4

tests: Add cases for FCI interpolation methods

aee8ecc

tests: Increase nx for hermitespline interpolation boundary problem

8e9c0fc

tests: Add monotonichermitespline, decrease resolution scan

6a4204f

tests: Small refactor of FCI tests

760a24f

Apply black changes

58e2673

ZedThree requested a review from dschwoerer November 4, 2025 11:50

Apply clang-format changes

10b0d40

github-actions bot reviewed Nov 4, 2025

View reviewed changes

dschwoerer previously approved these changes Nov 4, 2025

View reviewed changes

Apply clang-tidy fixes to FCI tests

ad30f00

Fix comment formatting

f17d21a

ZedThree dismissed dschwoerer’s stale review via f17d21a November 4, 2025 16:15

github-actions bot reviewed Nov 6, 2025

View reviewed changes

src/mesh/parallel/fci.cxx Outdated Show resolved Hide resolved

ZedThree and others added 3 commits November 6, 2025 17:27

Apply suggestion from @dschwoerer

783968a

Co-authored-by: David Bold <[email protected]>

tests: Fix typo that made tests always pass

c8faf01

tests: Remove monotonichermitespline FCI case

3dbed9a

ZedThree force-pushed the fci-tests branch from 952a0e9 to 3dbed9a Compare November 6, 2025 17:29

ZedThree mentioned this pull request Nov 7, 2025

Port some Hermes-3 finite volume operators #3200

Open

Add rtol, atol to MonotonicHermiteSpline

9f78a3c

This allows the user to control the clipping, allowing some overshoot Also restore the FCI MMS test with this interpolator

github-actions bot reviewed Nov 21, 2025

View reviewed changes

		// TODO(peter): Can we remove this if we apply (dirichlet?) BCs to
		// the X derivatives? Note that we need at least _2_

		// TODO(peter): Should we apply dirichlet BCs to derivatives?

Add more tests for FCI operators #3196

Are you sure you want to change the base?

Add more tests for FCI operators #3196

Uh oh!

Conversation

ZedThree commented Nov 4, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dschwoerer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZedThree commented Nov 6, 2025

Uh oh!

bendudson commented Nov 6, 2025

Uh oh!

dschwoerer commented Nov 7, 2025

Uh oh!

dschwoerer commented Nov 7, 2025

Uh oh!

ZedThree commented Nov 7, 2025

Uh oh!

ZedThree commented Nov 21, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants