Skip to content
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d26b825
Track yoffset in SpecificInd
dschwoerer Mar 18, 2025
b27d19d
Track yoffset in Field3D
dschwoerer Mar 18, 2025
aaf102d
Add some checks to ensure the parallel offset is correct
dschwoerer Mar 18, 2025
feec777
Automatically select the appropriate parallel slice
dschwoerer Mar 18, 2025
49bae21
Apply clang-format changes
dschwoerer Mar 18, 2025
247c2fb
Fix: preserve yoffset with other operations
dschwoerer Mar 19, 2025
6e834db
Add a version of `.i.yp()` that does not shift the parallel field
dschwoerer Mar 19, 2025
242b9b2
Add y-shifted regions by default
dschwoerer Mar 19, 2025
53c9e31
Fix hermite_spline_z for auto-shift
dschwoerer Mar 19, 2025
fe888ae
Avoid warning with 3D metrics
dschwoerer Mar 19, 2025
76dc241
Apply clang-format changes
dschwoerer Mar 19, 2025
eeb7c14
Ajust tests for auto parallel slices
dschwoerer Jun 2, 2025
980f00b
Merge branch 'fci-auto-with-debug-higher-order' into yup-ydown-auto
dschwoerer Jul 21, 2025
fa1bcb4
Apply suggestions from code review
dschwoerer Jul 21, 2025
79e2579
Apply clang-format changes
dschwoerer Jul 21, 2025
b714f7a
Add header explicitly for clang-tidy
dschwoerer Jul 21, 2025
742e6d0
Fixup `suggestions from code-review`
dschwoerer Jul 21, 2025
58caf56
Apply clang-format changes
dschwoerer Jul 21, 2025
6f8a4c9
Remove extra } from code review
dschwoerer Jul 21, 2025
159c26e
Apply clang-format changes
dschwoerer Jul 21, 2025
9496211
Add yoffset to emptyFrom
dschwoerer Jul 21, 2025
9b052eb
Use correct yoffset for accessing parallel fields
dschwoerer Jul 21, 2025
085948c
Fix mock fields to have realistic yoffset
dschwoerer Jul 21, 2025
288eb77
Apply clang-format changes
dschwoerer Jul 21, 2025
f3fd242
Disable hypre3d test
dschwoerer Jul 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions include/bout/field3d.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class Field3D;
#include "bout/array.hxx"
#include "bout/assert.hxx"
#include "bout/bout_types.hxx"
#include "bout/boutexception.hxx"
#include "bout/field.hxx"
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 boutexception.hxx is not used directly [misc-include-cleaner]

Suggested change
#include "bout/field.hxx"
#include "bout/field.hxx"

#include "bout/field2d.hxx"
#include "bout/fieldperp.hxx"
Expand Down Expand Up @@ -271,23 +272,27 @@ public:
/// Return reference to yup field
Field3D& yup(std::vector<Field3D>::size_type index = 0) {
ASSERT2(index < yup_fields.size());
ASSERT2(yup_fields[index].yoffset == static_cast<int>(index) + 1);
return yup_fields[index];
}
/// Return const reference to yup field
const Field3D& yup(std::vector<Field3D>::size_type index = 0) const {
ASSERT2(index < yup_fields.size());
ASSERT2(yup_fields[index].yoffset == static_cast<int>(index) + 1);
return yup_fields[index];
}

/// Return reference to ydown field
Field3D& ydown(std::vector<Field3D>::size_type index = 0) {
ASSERT2(index < ydown_fields.size());
ASSERT2(ydown_fields[index].yoffset == -1 - static_cast<int>(index));
return ydown_fields[index];
}

/// Return const reference to ydown field
const Field3D& ydown(std::vector<Field3D>::size_type index = 0) const {
ASSERT2(index < ydown_fields.size());
ASSERT2(ydown_fields[index].yoffset == -1 - static_cast<int>(index));
return ydown_fields[index];
}

Expand Down Expand Up @@ -351,8 +356,27 @@ public:
return std::end(getRegion("RGN_ALL"));
};

BoutReal& operator[](const Ind3D& d) { return data[d.ind]; }
const BoutReal& operator[](const Ind3D& d) const { return data[d.ind]; }
BoutReal& operator[](const Ind3D& d) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It has been a long time since I've looked at this code but could this be return ynext(d.yoffset - yoffset)[d]? One could then still include some of the consistency checking but it would move some of the logic into CHECK regions that disappear in production runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return ynext(d.yoffset - yoffset)[d] would always call ynext - even if this already the right field, and thus would fail.

The issue is that both the main field, which contains parallel fields, as well as the parallel fields, are of type Field3D.

We could (together with PR #3149) only enable this for Field3DParallel. There might be subtle bugs, where it depends on the type (Field3DParallel/Field3D) on whether it uses the correct indexing. But we could add those with higher CHECK level, so that should be fine.

if (d.yoffset != 0) {
if (yoffset == 0) {
if (hasParallelSlices()) {
return ynext(d.yoffset)[d];
}
#if CHECK >= 2
if (isFci()) { // We probably should assert here that this is field aligned
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be if ( ! hasParallelSlices() && isFci()) and placed before the return line so the return doesn't need to be guarded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would still need to be guarded - because for FA there are no parallel slices, and thus we need to use the return at the end of the function.

throw BoutException(
"Tried to access parallel slices, but they are not calculated!");
}
}
#endif
} else {
ASSERT2(d.yoffset == yoffset);
}
}
return data[d.ind];
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: expected member name or ';' after declaration specifiers [clang-diagnostic-error]

    return data[d.ind];
    ^

} const BoutReal& operator[](const Ind3D& d) const {
return (*const_cast<Field3D*>(this))[d];
}

BoutReal& operator()(const IndPerp& d, int jy);
const BoutReal& operator()(const IndPerp& d, int jy) const;
Expand Down Expand Up @@ -547,6 +571,10 @@ private:
template <class T>
Options* track(const T& change, std::string operation);
Options* track(const BoutReal& change, std::string operation);

public:
int yoffset{0};
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 'yoffset' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int yoffset{0};
      ^

void setParallelRegions();
};

// Non-member overloaded operators
Expand Down
23 changes: 19 additions & 4 deletions include/bout/region.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,12 @@ template <IND_TYPE N>
struct SpecificInd {
int ind = -1; ///< 1D index into Field
int ny = -1, nz = -1; ///< Sizes of y and z dimensions
int yoffset = 0;

SpecificInd() = default;
SpecificInd(int i, int ny, int nz) : ind(i), ny(ny), nz(nz){};
SpecificInd(int i, int ny, int nz, int yoffset)
: ind(i), ny(ny), nz(nz), yoffset(yoffset){};
explicit SpecificInd(int i) : ind(i){};

/// Allow explicit conversion to an int
Expand Down Expand Up @@ -275,7 +278,9 @@ struct SpecificInd {
}
}

inline SpecificInd xp(int dx = 1) const { return {ind + (dx * ny * nz), ny, nz}; }
inline SpecificInd xp(int dx = 1) 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: function 'xp' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]

Suggested change
inline SpecificInd xp(int dx = 1) const {
SpecificInd xp(int dx = 1) const {

return {ind + (dx * ny * nz), ny, nz, yoffset};
}
/// The index one point -1 in x
inline SpecificInd xm(int dx = 1) const { return xp(-dx); }
/// The index one point +1 in y
Expand All @@ -286,8 +291,18 @@ struct SpecificInd {
}
#endif
ASSERT3(std::abs(dy) < ny);
return {ind + (dy * nz), ny, nz};
return {ind + (dy * nz), ny, nz, yoffset + dy};
}
inline SpecificInd yp_no_parallel_shift(int dy = 1) 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: function 'yp_no_parallel_shift' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]

Suggested change
inline SpecificInd yp_no_parallel_shift(int dy = 1) const {
SpecificInd yp_no_parallel_shift(int dy = 1) const {

#if CHECK >= 4
if (y() + dy < 0 or y() + dy >= ny) {
throw BoutException("Offset in y ({:d}) would go out of bounds at {:d}", dy, ind);
}
#endif
ASSERT3(std::abs(dy) < ny);
return {ind + (dy * nz), ny, nz, yoffset};
}

/// The index one point -1 in y
inline SpecificInd ym(int dy = 1) const { return yp(-dy); }
/// The index one point +1 in z. Wraps around zend to zstart
Expand All @@ -297,7 +312,7 @@ struct SpecificInd {
inline SpecificInd zp(int dz = 1) const {
ASSERT3(dz >= 0);
dz = dz <= nz ? dz : dz % nz; //Fix in case dz > nz, if not force it to be in range
return {(ind + dz) % nz < dz ? ind - nz + dz : ind + dz, ny, nz};
return {(ind + dz) % nz < dz ? ind - nz + dz : ind + dz, ny, nz, yoffset};
}
/// The index one point -1 in z. Wraps around zstart to zend
/// An alternative, non-branching calculation is :
Expand All @@ -306,7 +321,7 @@ struct SpecificInd {
inline SpecificInd zm(int dz = 1) const {
dz = dz <= nz ? dz : dz % nz; //Fix in case dz > nz, if not force it to be in range
ASSERT3(dz >= 0);
return {(ind) % nz < dz ? ind + nz - dz : ind - dz, ny, nz};
return {(ind) % nz < dz ? ind + nz - dz : ind - dz, ny, nz, yoffset};
}
/// Automatically select zm or zp depending on sign
inline SpecificInd zpm(int dz) const { return dz > 0 ? zp(dz) : zm(-dz); }
Expand Down
18 changes: 16 additions & 2 deletions src/field/field3d.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ Field3D::Field3D(Mesh* localmesh, CELL_LOC location_in, DirectionTypes direction
/// later)
Field3D::Field3D(const Field3D& f)
: Field(f), data(f.data), yup_fields(f.yup_fields), ydown_fields(f.ydown_fields),
regionID(f.regionID) {
regionID(f.regionID), yoffset(f.yoffset) {

TRACE("Field3D(Field3D&)");

Expand Down Expand Up @@ -157,7 +157,9 @@ void Field3D::splitParallelSlices() {
// Note the fields constructed here will be fully overwritten by the
// ParallelTransform, so we don't need a full constructor
yup_fields.emplace_back(fieldmesh);
yup_fields[i].yoffset = i + 1;
ydown_fields.emplace_back(fieldmesh);
ydown_fields[i].yoffset = -1 - i;
if (isFci()) {
yup_fields[i].setRegion(fmt::format("RGN_YPAR_{:+d}", i + 1));
ydown_fields[i].setRegion(fmt::format("RGN_YPAR_{:+d}", -i - 1));
Expand Down Expand Up @@ -224,6 +226,15 @@ bool Field3D::requiresTwistShift(bool twist_shift_enabled) {
getDirectionY());
}

void Field3D::setParallelRegions() {
for (int i = 0; i < fieldmesh->ystart; ++i) {
yup_fields[i].yoffset = i + 1;
yup_fields[i].setRegion(fmt::format("RGN_YPAR_{:+d}", i + 1));
ydown_fields[i].yoffset = -1 - i;
ydown_fields[i].setRegion(fmt::format("RGN_YPAR_{:+d}", -1 - i));
}
}

// Not in header because we need to access fieldmesh
BoutReal& Field3D::operator()(const IndPerp& d, int jy) {
return operator[](fieldmesh->indPerpto3D(d, jy));
Expand Down Expand Up @@ -277,6 +288,7 @@ Field3D& Field3D::operator=(const Field3D& rhs) {
yup_fields = rhs.yup_fields;
ydown_fields = rhs.ydown_fields;
regionID = rhs.regionID;
yoffset = rhs.yoffset;

// Copy the data and data sizes
nx = rhs.nx;
Expand All @@ -295,12 +307,13 @@ Field3D& Field3D::operator=(Field3D&& rhs) {
// Move parallel slices or delete existing ones.
yup_fields = std::move(rhs.yup_fields);
ydown_fields = std::move(rhs.ydown_fields);
regionID = rhs.regionID;
yoffset = rhs.yoffset;

// Move the data and data sizes
nx = rhs.nx;
ny = rhs.ny;
nz = rhs.nz;
regionID = rhs.regionID;

data = std::move(rhs.data);

Expand Down Expand Up @@ -896,6 +909,7 @@ void swap(Field3D& first, Field3D& second) noexcept {
swap(first.deriv, second.deriv);
swap(first.yup_fields, second.yup_fields);
swap(first.ydown_fields, second.ydown_fields);
swap(first.yoffset, second.yoffset);
}

const Region<Ind3D>&
Expand Down
4 changes: 4 additions & 0 deletions src/field/gen_fieldops.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
{% if lhs == rhs == "Field3D" %}
{{out.name}}.setRegion({{lhs.name}}.getMesh()->getCommonRegion({{lhs.name}}.getRegionID(),
{{rhs.name}}.getRegionID()));
ASSERT2({{lhs.name}}.yoffset == {{rhs.name}}.yoffset);
{{out.name}}.yoffset = {{lhs.name}}.yoffset;
#if BOUT_USE_FCI_AUTOMAGIC
if ({{lhs.name}}.isFci() and {{lhs.name}}.hasParallelSlices() and {{rhs.name}}.hasParallelSlices()) {
{{out.name}}.splitParallelSlices();
Expand All @@ -23,6 +25,7 @@
#endif
{% elif lhs == "Field3D" %}
{{out.name}}.setRegion({{lhs.name}}.getRegionID());
{{out.name}}.yoffset = {{lhs.name}}.yoffset;
{% if rhs == "BoutReal" %}
#if BOUT_USE_FCI_AUTOMAGIC
if ({{lhs.name}}.isFci() and {{lhs.name}}.hasParallelSlices()) {
Expand All @@ -36,6 +39,7 @@
{% endif %}
{% elif rhs == "Field3D" %}
{{out.name}}.setRegion({{rhs.name}}.getRegionID());
{{out.name}}.yoffset = {{rhs.name}}.yoffset;
{% if lhs == "BoutReal" %}
#if BOUT_USE_FCI_AUTOMAGIC
if ({{rhs.name}}.isFci() and {{rhs.name}}.hasParallelSlices()) {
Expand Down
1 change: 1 addition & 0 deletions src/field/gen_fieldops.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ def smart_open(filename, mode="r"):
)

header = """// This file is autogenerated - see gen_fieldops.py
#include <bout/assert.hxx>
#include <bout/field2d.hxx>
#include <bout/field3d.hxx>
#include <bout/globals.hxx>
Expand Down
Loading