Add more tests for FCI operators by ZedThree · Pull Request #3196 · boutproject/BOUT-dev

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

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

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

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); ^

- 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.

Co-authored-by: David Bold <dschwoerer@users.noreply.github.com>

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

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

github-actions bot reviewed Nov 4, 2025

View reviewed changes

dschwoerer previously approved these changes Nov 4, 2025

View reviewed changes

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 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

github-actions bot reviewed Nov 21, 2025

View reviewed changes

ZedThree and others added 16 commits December 3, 2025 14:25

tests: Ruff fixes for FCI runtest

74b736d

tests: Expand FCI MMS test to Grad2_par2

eb589e1

tests: Generalise FCI MMS test to allow for more cases

c0cc84b

tests: Add test for FCI Div_par

2eda48c

tests: Make MMS script update input in-place

636bf28

tests: Add test for FCI Div_par_K_Grad_par

16318ec

tests: Add test for FCI Laplace_par

2f83692

Reduce duplication in FCI mms script

0cde140

Remove circular include

37dc273

tests: Add cases for FCI interpolation methods

9ac46e8

tests: Increase nx for hermitespline interpolation boundary problem

5974a6e

tests: Add monotonichermitespline, decrease resolution scan

6ab945e

tests: Small refactor of FCI tests

16c3718

Apply black changes

3d3ff65

ZedThree and others added 7 commits December 3, 2025 14:28

Apply clang-format changes

79f9d0f

Apply clang-tidy fixes to FCI tests

cee3e44

Fix comment formatting

88d8db8

Apply suggestion from @dschwoerer

9d9922c

Co-authored-by: David Bold <dschwoerer@users.noreply.github.com>

tests: Fix typo that made tests always pass

e961e75

tests: Remove monotonichermitespline FCI case

2674a58

Add rtol, atol to MonotonicHermiteSpline

f00db39

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

ZedThree force-pushed the fci-tests branch from 9f78a3c to f00db39 Compare December 3, 2025 14:33

Add missing header

0efe4e3

ZedThree requested a review from dschwoerer December 3, 2025 14:34

		// 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?

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!

3 participants