Skip to content

Conversation

@dschwoerer
Copy link
Contributor

@dschwoerer dschwoerer commented Nov 6, 2025

Here a summary of the changes:

Field3DParallel

A Field3DParallel is essentially a Field3D, that preserves the parallel slices.
This allows to avoid unnecessary computation of of parallel fields, that are not needed.
It also ensures parallel slices are present (for FCI) when later parallel derivatives are computed.
So any function that will call DDY later on, can change the signature to Field3DParallel to declare that parallel slices need to be present for FCI.

New Y-Boundary Operators

Custom sheath boundary conditions can now be implemented using an abstraction. The documentation is on RTD
That allows having one code path for FCI and FA, for upper and lower sheath BC, all in one place.

Various small fixups

loadParallelMetrics

For FCI the parallel slices of the metric components are loaded, as they are sometimes needed.
This makes FCI essentially 3D only.
There is now also the option to say a field is not allow to compute the parallel slices, to avoid overwriting them: allowCalcParallelSlices.

Tracking on failure

Only if the simulation fails to evolve (Currently euler and pvode only):
The different components of timederivate are dumped to a BOUT.debug.*.nc which can be handy for figuring out which term is causing the instability. No more having to re-run the simulation with some terms disabled to debug such issues. This causes (an unkown) memory overhead - but ONLY if the solver has failed already.

More names of parallel boundary region + cleanup of code

Support things like bndry_par_xin or bndry_par_yup.

Add monotonichermitespline for all cases

Merged into hermitespline and works also in parallel.

Support several parallel slices for FCI

MYG=2 is now supported.

Previously only the first boundary point was set
For FCI we need to be able to access "random" data from the adjacent
slices. If they are split in x-direction, this requires some tricky
communication pattern.

It can be used like this:
```
// Create object
GlobalField3DAccess fci_comm(thismesh);
// let it know what data points will be required:
// where IndG3D is an index in the global field, which would be the
// normal Ind3D if there would be only one proc.
fci_comm.get(IndG3D(i, ny, nz));
// If all index have been added, the communication pattern will be
// established. This has to be called by all processors in parallel
fci_comm.setup()
// Once the data for a given field is needed, it needs to be
// communicated:
GlobalField3DAccessInstance global_data = fci_comm.communicate(f3d);
// and can be accessed like this
BoutReal data = global_data[IndG3D(i, ny, nz)];
// ny and nz in the IndG3D are always optional.
```
If they are two instances of the same template, this allows to have an
if in the inner loop that can be optimised out.
We have the info, so might as well store it
Taken from hermes-3, adopted for higher order
lower_bound takes into account the data is sorted
Ensures we all ways check for monotonicity
Tags were different for sender and receiver
Otherwise mpi might wait for the wrong request.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 219. Check the log or trigger a new build to see more.

******************************************************************/

#define PVODE_BAND_ELEM(A,i,j) ((A->data)[j][i-j+(A->smu)])
#define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)])
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'PVODE_BAND_ELEM' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)])
        ^

******************************************************************/

#define PVODE_BAND_COL(A,j) (((A->data)[j])+(A->smu))
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
        ^

******************************************************************/

#define BAND_COL(A, j) (((A->data)[j]) + (A->smu))
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
        ^

#include <algorithm>
#include <functional>

class BoundaryRegionIter {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'BoundaryRegionIter' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class BoundaryRegionIter {
      ^

inline T toFieldAligned(const T& f, const std::string& region = "RGN_ALL") {
static_assert(bout::utils::is_Field_v<T>, "toFieldAligned only works on Fields");
ASSERT3(f.getCoordinates() != nullptr);
return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member access into incomplete type 'Coordinates' [clang-diagnostic-error]

  return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region);
                           ^
Additional context

include/bout/index_derivs_interface.hxx:218: in instantiation of function template specialization 'toFieldAligned' requested here

    const T f_aligned = is_unaligned ? toFieldAligned(f, "RGN_NOX") : f;
                                       ^

include/bout/index_derivs_interface.hxx:227: in instantiation of function template specialization 'bout::derivatives::index::DDY' requested here

  return DDY(f.asField3D(), outloc, method, region);
         ^

include/bout/field_data.hxx:42: forward declaration of 'Coordinates'

class Coordinates;
      ^


int tracking_state{0};
Options* tracking{nullptr};
std::string selfname;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'selfname' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  std::string selfname;
              ^

explicit Field3DParallel(Types... args) : Field3D(std::move(args)...) {
ensureFieldAligned();
}
Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]

Suggested change
Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Field3DParallel(const Field3D& f) : Field3D(f) { ensureFieldAligned(); }

ensureFieldAligned();
}
Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]

Suggested change
Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Field3DParallel(const Field2D& f) : Field3D(f) { ensureFieldAligned(); }

};

Field3DParallel Field3D::asField3DParallel() { return Field3DParallel(*this); }
const Field3DParallel Field3D::asField3DParallel() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: return type 'const Field3DParallel' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]

include/bout/field3d.hxx:533:

-   inline const Field3DParallel asField3DParallel() const;
+   inline Field3DParallel asField3DParallel() const;
Suggested change
const Field3DParallel Field3D::asField3DParallel() const {
Field3DParallel Field3D::asField3DParallel() const {


inline Field3DParallel
filledFrom(const Field3DParallel& f,
std::function<BoutReal(int yoffset, Ind3D index)> func) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the parameter 'func' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change
std::function<BoutReal(int yoffset, Ind3D index)> func) {
const std::function<BoutReal(int yoffset, Ind3D index)>& func) {

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 219. Check the log or trigger a new build to see more.

******************************************************************/

#define PVODE_BAND_ELEM(A,i,j) ((A->data)[j][i-j+(A->smu)])
#define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)])
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'PVODE_BAND_ELEM' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)])
        ^

******************************************************************/

#define PVODE_BAND_COL(A,j) (((A->data)[j])+(A->smu))
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
        ^

******************************************************************/

#define BAND_COL(A, j) (((A->data)[j]) + (A->smu))
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
        ^

#include <algorithm>
#include <functional>

class BoundaryRegionIter {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'BoundaryRegionIter' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class BoundaryRegionIter {
      ^

inline T toFieldAligned(const T& f, const std::string& region = "RGN_ALL") {
static_assert(bout::utils::is_Field_v<T>, "toFieldAligned only works on Fields");
ASSERT3(f.getCoordinates() != nullptr);
return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member access into incomplete type 'Coordinates' [clang-diagnostic-error]

  return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region);
                           ^
Additional context

include/bout/index_derivs_interface.hxx:218: in instantiation of function template specialization 'toFieldAligned' requested here

    const T f_aligned = is_unaligned ? toFieldAligned(f, "RGN_NOX") : f;
                                       ^

include/bout/index_derivs_interface.hxx:227: in instantiation of function template specialization 'bout::derivatives::index::DDY' requested here

  return DDY(f.asField3D(), outloc, method, region);
         ^

include/bout/field_data.hxx:42: forward declaration of 'Coordinates'

class Coordinates;
      ^


int tracking_state{0};
Options* tracking{nullptr};
std::string selfname;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'selfname' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  std::string selfname;
              ^

explicit Field3DParallel(Types... args) : Field3D(std::move(args)...) {
ensureFieldAligned();
}
Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]

Suggested change
Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Field3DParallel(const Field3D& f) : Field3D(f) { ensureFieldAligned(); }

ensureFieldAligned();
}
Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]

Suggested change
Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); }
Field3DParallel(const Field2D& f) : Field3D(f) { ensureFieldAligned(); }

};

Field3DParallel Field3D::asField3DParallel() { return Field3DParallel(*this); }
const Field3DParallel Field3D::asField3DParallel() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: return type 'const Field3DParallel' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]

include/bout/field3d.hxx:533:

-   inline const Field3DParallel asField3DParallel() const;
+   inline Field3DParallel asField3DParallel() const;
Suggested change
const Field3DParallel Field3D::asField3DParallel() const {
Field3DParallel Field3D::asField3DParallel() const {


inline Field3DParallel
filledFrom(const Field3DParallel& f,
std::function<BoutReal(int yoffset, Ind3D index)> func) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the parameter 'func' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change
std::function<BoutReal(int yoffset, Ind3D index)> func) {
const std::function<BoutReal(int yoffset, Ind3D index)>& func) {

@ZedThree
Copy link
Member

Please can you split this into one PR per change? It's 100 files and 5k lines of changes, it's going to be very difficult to review. GitHub will only show a single file at once.

@dschwoerer
Copy link
Contributor Author

Sure!
Is ready for re-review / merge: #2889
I will keep working on the other bits ...

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 288. Check the log or trigger a new build to see more.

const FieldMetric& Jxz() const;

private:
mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

include/bout/coordinates.hxx:40:

+ #include <optional>


private:
mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh;
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: declaration uses identifier '_Jxz_centre', which is a reserved identifier [bugprone-reserved-identifier]

Suggested change
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre;
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, Jxz_centre;


private:
mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh;
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: declaration uses identifier '_Jxz_yhigh', which is a reserved identifier [bugprone-reserved-identifier]

Suggested change
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre;
mutable std::optional<FieldMetric> _Jxz_ylow, Jxz_yhigh, _Jxz_centre;


private:
mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh;
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: declaration uses identifier '_Jxz_ylow', which is a reserved identifier [bugprone-reserved-identifier]

Suggested change
mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre;
mutable std::optional<FieldMetric> Jxz_ylow, _Jxz_yhigh, _Jxz_centre;


template <typename T>
auto& getStore() {
if constexpr (std::is_same<T, Field3DParallel>::value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

include/bout/deriv_store.hxx:37:

- #include <bout/scorepwrapper.hxx>
+ #include "bout/field3d.hxx"
+ #include <bout/scorepwrapper.hxx>

#include <bout/mesh.hxx>
#if CHECK > 0
#include <bout/output_bout_types.hxx>
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header output_bout_types.hxx is not used directly [misc-include-cleaner]

Suggested change
#endif
#endif

// Left and right cell face values
BoutReal L, R;
/// Cell centre values
BoutReal c;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

include/bout/fv_ops.hxx:7:

- #include "bout/build_defines.hxx"
+ #include "bout/bout_types.hxx"
+ #include "bout/build_defines.hxx"

BoutReal m;
BoutReal p;
BoutReal mm = BoutNaN;
BoutReal pp = BoutNaN;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

 BoutNaN;
 ^

BoutReal flux = NAN;

if (mesh->lastY(i) && (j == mesh->yend) && !mesh->periodicY(i)) {
if (is_last_y && (j == mesh->yend) && !is_periodic_y) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Real flux = NAN;
            ^

const auto& v_down = v_in.ydown();

Field3D result{emptyFrom(f_in)};
BOUT_FOR(i, f_in.getRegion("RGN_NOBNDRY")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

emptyFrom(f_in)};
^

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants