Refactor coordinates #2873

tomc271 · 2024-02-23T14:07:26Z

Encapsulate all private members of Coordinates class.
New classes for metric tensor, Christoffel symbols.

examples/2Dturbulence_multigrid/esel.cxx

examples/6field-simple/elm_6f.cxx

bendudson · 2024-02-23T16:24:43Z

Thanks @tomc271 ! This is quite an epic amount of work.

ZedThree

Thanks @tomc271! I've made a bunch of comments on things to work through. A couple of big things though:

this has diverged from next a bit, so that will need merging in and conflicts resolving. There's been some important bug fixes and CI changes in next that would be good to get in ASAP
some of the changes haven't propagated through to some subsystems, such as the various PETSc laplace operators. You will need to either build against PETSc locally to find them, or use some searching to find the remaining places

include/bout/mesh.hxx

src/mesh/coordinates_accessor.cxx

tools/pylib/_boutpp_build/boutcpp.pxd.jinja

include/bout/mesh.hxx

src/mesh/mesh.cxx

src/mesh/christoffel_symbols.cxx

src/mesh/coordinates.cxx

examples/6field-simple/elm_6f.cxx

…ers.

…ties.

…t be marked explicit to avoid unintentional implicit conversions.

…ometry().

…FakeMesh has no 'source', which causes a crash when the constructor leads to initialisation of FieldMetric objects).

…reference.

* next: (72 commits) Make function signature match implementation in example Fix some clang-tidy issues in dalf3 Apply suggestions from code review Apply clang-format changes prefer `auto *` for pointer types Fix: preserve regionID Fix exception message Ensure pointer is checked before dereferencing Try to do the right thing for linking netCDF::netcdf Add LC gitlab CI for GPU build/run tests BoutMask: Add non-const operator[](Ind3D) Use consistently signed char Apply clang-format changes naulin laplace: Acceptance tolerances after maxits CI: Remove test of deleted example CI: Bump all ubuntu images Read 2D variables into Field3D Fix "call to deleted constructor of 'Options' [clang-diagnostic-error]" Apply clang-format changes Revert change to reference for 'datadir'. ...

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-23T18:38:34Z

src/mesh/coordinates.cxx

-  }
-}
+// Utility function for fixing up guard cells of zShift
+void Coordinates::fixZShiftGuards(Field2D& zShift) const {


warning: method 'fixZShiftGuards' can be made static [readability-convert-member-functions-to-static]

include/bout/coordinates.hxx:503:

- void fixZShiftGuards(Field2D& zShift) const; + static void fixZShiftGuards(Field2D& zShift) ;

Suggested change

void Coordinates::fixZShiftGuards(Field2D& zShift) const {

void Coordinates::fixZShiftGuards(Field2D& zShift) {

github-actions · 2024-10-23T18:38:34Z

src/mesh/difops.cxx


          BoutReal gp, gm;

          // Vx = DDZ(f)
-          BoutReal vx = (f(x, y, zp) - f(x, y, zm)) / (2. * metric->dz(x, y, z));
+          BoutReal const vx = (f(x, y, zp) - f(x, y, zm)) / (2. * metric->dz(x, y, z));


warning: variable name 'vx' is too short, expected at least 3 characters [readability-identifier-length]

BoutReal const vx = (f(x, y, zp) - f(x, y, zm)) / (2. * metric->dz(x, y, z)); ^

github-actions · 2024-10-23T18:38:34Z

src/mesh/fv_ops.cxx


        // Calculate gradients on cell faces -- assumes constant grid spacing

-        BoutReal gR =
+        BoutReal const gR =
            (coords->g11(i, j, k) + coords->g11(i + 1, j, k))


warning: variable name 'gR' is too short, expected at least 3 characters [readability-identifier-length]

st gR = ^

github-actions · 2024-10-23T18:38:34Z

src/mesh/fv_ops.cxx

            (coords->g11(i, j, k) + coords->g11(i + 1, j, k))
                * (f(i + 1, j, k) - f(i, j, k))
                / (coords->dx(i + 1, j, k) + coords->dx(i, j, k))
            + 0.5 * (coords->g13(i, j, k) + coords->g13(i + 1, j, k))
                  * (f(i + 1, j, kp) - f(i + 1, j, km) + f(i, j, kp) - f(i, j, km))
                  / (4. * coords->dz(i, j, k));

-        BoutReal gL =
+        BoutReal const gL =
            (coords->g11(i - 1, j, k) + coords->g11(i, j, k))


warning: variable name 'gL' is too short, expected at least 3 characters [readability-identifier-length]

st gL = ^

github-actions · 2024-10-23T18:38:34Z

src/mesh/fv_ops.cxx

            (coords->g11(i - 1, j, k) + coords->g11(i, j, k))
                * (f(i, j, k) - f(i - 1, j, k))
                / (coords->dx(i - 1, j, k) + coords->dx(i, j, k))
            + 0.5 * (coords->g13(i - 1, j, k) + coords->g13(i, j, k))
                  * (f(i - 1, j, kp) - f(i - 1, j, km) + f(i, j, kp) - f(i, j, km))
                  / (4 * coords->dz(i, j, k));

-        BoutReal gD =
+        BoutReal const gD =
            coords->g13(i, j, k)


warning: variable name 'gD' is too short, expected at least 3 characters [readability-identifier-length]

st gD = ^

github-actions · 2024-10-23T18:38:34Z

src/mesh/fv_ops.cxx

            coords->g13(i, j, k)
                * (f(i + 1, j, km) - f(i - 1, j, km) + f(i + 1, j, k) - f(i - 1, j, k))
                / (4. * coords->dx(i, j, k))
            + coords->g33(i, j, k) * (f(i, j, k) - f(i, j, km)) / coords->dz(i, j, k);

-        BoutReal gU =
+        BoutReal const gU =
            coords->g13(i, j, k)


warning: variable name 'gU' is too short, expected at least 3 characters [readability-identifier-length]

st gU = ^

github-actions · 2024-10-23T18:38:34Z

tests/MMS/GBS/gbs.cxx

-  dz4 = SQ(SQ(coords->dz));
+  dx4 = SQ(SQ(coords->dx()));
+  dy4 = SQ(SQ(coords->dy()));
+  dz4 = SQ(SQ(coords->dz()));


warning: assigning to 'BoutReal' (aka 'double') from incompatible type 'Field2D' [clang-diagnostic-error]

dz4 = SQ(SQ(coords->dz())); ^

Location will already be set from initialising using `emptyFrom`

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-24T17:13:08Z

src/mesh/coordinates.cxx

-  }
-}
+// Utility function for fixing up guard cells of zShift
+void Coordinates::fixZShiftGuards(Field2D& zShift) const {


warning: method 'fixZShiftGuards' can be made static [readability-convert-member-functions-to-static]

include/bout/coordinates.hxx:479:

- void fixZShiftGuards(Field2D& zShift) const; + static void fixZShiftGuards(Field2D& zShift) ;

Suggested change

void Coordinates::fixZShiftGuards(Field2D& zShift) const {

void Coordinates::fixZShiftGuards(Field2D& zShift) {

github-actions · 2024-10-24T17:13:08Z

src/mesh/mesh.cxx

+    *coords_map[location] = std::move(new_coordinates);
+
+    auto recalculate_staggered = false;
+    new_coordinates.recalculateAndReset(recalculate_staggered,


warning: 'new_coordinates' used after it was moved [bugprone-use-after-move]

new_coordinates.recalculateAndReset(recalculate_staggered, ^

Additional context

src/mesh/mesh.cxx:790: move occurred here

*coords_map[location] = std::move(new_coordinates); ^

github-actions · 2024-10-24T17:13:08Z

tests/integrated/test-snb/test_snb.cxx

-        double yn = (double(y) + 0.5) / double(mesh->yend + 1);
+    {
+      auto dy = emptyFrom(coord->dx());
+      auto J = emptyFrom(coord->J());


warning: variable name 'J' is too short, expected at least 3 characters [readability-identifier-length]

auto J = emptyFrom(coord->J()); ^

tomc271 requested a review from ZedThree February 23, 2024 14:07

bendudson reviewed Feb 23, 2024

View reviewed changes

examples/2Dturbulence_multigrid/esel.cxx Outdated Show resolved Hide resolved

bendudson reviewed Feb 23, 2024

View reviewed changes

examples/6field-simple/elm_6f.cxx Outdated Show resolved Hide resolved

ZedThree reviewed Feb 29, 2024

View reviewed changes

tomchapman added 25 commits March 2, 2024 15:26

Update documentation.

904546b

Make parameters to Coordinates constructor const references.

dc82c8b

Prefer const.

924618d

Remove unused setters for Christoffel symbol terms.

ab2e122

Make dx, dy, dz private.

07a2862

Make dx, dy, dz private in cython interface.

294737b

Update more tests and examples to use new dx, dy, dz getters and sett…

84e108f

…ers.

Add setter for dy that takes [x, y] indices. Update test_snb.

54b343b

Corrected comment.

41b0a5b

Prefer const.

0106a2c

Make d1_dx, d1_dy, d1_dz private.

0bf195b

Let DataFileFacade::add() take a const value.

f248de9

Make non_uniform private.

6528658

Remove call to calculateGeometry() after setting metric tensor quanti…

3b3915e

…ties.

Clang-Tidy: Constructors that are callable with a single argument mus…

05ea951

…t be marked explicit to avoid unintentional implicit conversions.

Make ShiftTorsion and IntShiftTorsion private.

46a11ae

Clang-Tidy: Pass by value and use std::move().

62d19fb

Rename calculateGeometry() to communicateAndCheckMeshSpacing().

331eb1e

Remove comments referring to obsolete method Coordinates::calculateGe…

31c7640

…ometry().

Extract class ChristoffelSymbols.

6fe1813

Error handling: null check in Mesh::sourceHasYBoundaryGuards().

2cf9453

Add a constructor for ChristoffelSymbols that takes no Mesh (because …

3ccd720

…FakeMesh has no 'source', which causes a crash when the constructor leads to initialisation of FieldMetric objects).

Make 'function' parameter to map() and applyToComponents() methods a …

83c5d72

…reference.

Make parameters to metric tensor setters const references.

8c06cd1

Rename getters (and private fields) for metric tensor components.

b8e192d

boutproject deleted a comment from github-actions bot Oct 23, 2024

Fix clang-tidy const-correctness warnings

fa6663d

ZedThree mentioned this pull request Oct 23, 2024

Fix lots of clang-tidy issues in fv_ops #3007

Open

github-actions bot reviewed Oct 23, 2024

View reviewed changes

ZedThree added 17 commits October 24, 2024 09:33

Rename metric tensor component members

5b61d2b

Use std::array instead of C array

e2d3c52

Fix some clang-tidy warnings in metric_tensor

687f0c4

Reduce duplication in metric tensor between 2D/3D metric a little

2411ab2

Inline metric tensor function application map

1213035

Use free-function derivatives in metric tensor classes

e5010e4

Remove some unused ctors from metric tensor classes

a926df1

Ensure metric derivatives are always communicated

ff3fdef

Remove MetricTensor::setLocation

676f0a0

Location will already be set from initialising using `emptyFrom`

Pull out duplicated region name

ff12e4a

Remove specialised setDy, setJ overloads

8a1d040

Ensure grid spacings, Bxy, J, are always communicated

0663ea8

Ensure metric terms are always communicated

a4fe574

Simplify recalculateJacobian a little

b276d74

Eliminate some unnecessary local variables

61196bf

Inline some single-use functions

42dc6ee

Fix wrong type in MMS/gbs

47cff9e

ZedThree force-pushed the refactor-coordinates branch from f970905 to 47cff9e Compare October 24, 2024 16:36

github-actions bot reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor coordinates #2873

Refactor coordinates #2873

tomc271 commented Feb 23, 2024

bendudson commented Feb 23, 2024

ZedThree left a comment

github-actions bot left a comment

github-actions bot Oct 23, 2024

github-actions bot Oct 23, 2024

github-actions bot Oct 23, 2024

github-actions bot Oct 23, 2024

github-actions bot Oct 23, 2024

github-actions bot Oct 23, 2024

github-actions bot Oct 23, 2024

github-actions bot left a comment

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

	void Coordinates::fixZShiftGuards(Field2D& zShift) const {
	void Coordinates::fixZShiftGuards(Field2D& zShift) {

Refactor coordinates #2873

Are you sure you want to change the base?

Refactor coordinates #2873

Conversation

tomc271 commented Feb 23, 2024

bendudson commented Feb 23, 2024

ZedThree left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot Oct 23, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment