Refactor coordinates by tomc271 · Pull Request #2873 · boutproject/BOUT-dev

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

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

…void unintentional implicit conversions.

…ntMetricTensor in Coordinates::setBoundaryCells method, to avoid recalculation of the christoffel symbols, which would (at this stage) involve field data at different locations (specifically the metric tensor and jacobian).

…constructor that is used with FakeMesh.

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

otherwise when Coordinates setters call mesh->communicate() and that calls calcParallelSlices() and that calls getCoordinates() the coordinates field will be a null pointer.

…nd MetricTensor inverse() method Add method Coordinates::communicateMetricTensor() and call after constructing Coordinates to avoid circular dependency (between Mesh and Coordinates) as a result of the setter methods calling communicate()

to prevent getUniform(coords->zlength()) failing because the field is not const.

Update existing dy and J fields, to avoid invalid values in the guard cells.

…icate Fixes to address failing tests after changes to refactor-coordinates

…-coordinates-with-next-merged-in

Include changes from commit: "Return `std::optional` from `invert3x3`" SHA-1: c42dc24 which was not merged due to splitting files.

…erivatives::index::DDY() as the former checks canToFromFieldAligned().

This reverts commit e5010e4. Fix for failing fci tests.

tomc271 · 2025-09-30T12:57:47Z

Resolves #2128

dschwoerer · 2026-03-20T19:50:44Z

It was my understanding that you are going to re-enable to do coords->dy[i], instead of coords->dy()[i] - do you still plan to do that, or is that not planned?

That would not only make it mostly backwards compatible, but it would also reduce the diff hugely, as most of the BOUT++ code base would not need to be touched.

tomc271 · 2026-03-20T20:19:53Z

It was my understanding that you are going to re-enable to do coords->dy[i], instead of coords->dy()[i] - do you still plan to do that, or is that not planned?

That would not only make it mostly backwards compatible, but it would also reduce the diff hugely, as most of the BOUT++ code base would not need to be touched.

This summarises our thinking

dschwoerer · 2026-03-20T20:41:54Z

This summarises our thinking

I get the error that this link is invalid or has been deleted.

dschwoerer · 2026-03-20T20:49:35Z

This summarises our thinking

I get the error that this link is invalid or has been deleted.

It is working now with javascript enabled, google should fix there error messages.

This summarises our thinking
I would not call this "summarising" - this is a fairly length bit of text.

To summarise the link: Avoiding the explicit () call can cause issues with auto and templates - which can be very confusing, and as such should be avoided. Did I get that right?

dschwoerer · 2026-03-20T20:53:46Z

If so, you do not plan to do this in the future, and this PR is not a WIP and ready for review?
I asked 2 years ago #2873 (comment) - and would have started with a review if I would have known the answer.

Did you address #2873 (comment) ?

If you get this updated, I will try to review this (and #3046 )

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

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

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

aa42c9f

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

e44f1b7

Clang-Tidy: Single-argument constructors must be marked explicit to a…

721479d

…void unintentional implicit conversions.

Remove redundant 'mesh' parameter from Geometry constructor.

0d5c250

Formatting.

046a7ad

Remove Geometry class, as now redundant.

f2dc950

Corrected comment.

88e18b6

Ensure covariantMetricTensor is set.

0c65aed

Don't pass mesh to ChristoffelSymbols constructor in the Coordinates …

aa03305

…constructor that is used with FakeMesh.

Null check differential_operators in ChristoffelSymbols constructor.

25399d7

Ensure covariantMetricTensor is set.

3d81700

ZedThree added 7 commits October 24, 2024 13:41

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

bendudson mentioned this pull request Nov 16, 2024

Coordinates: Replacing sqrt(g_22) with JB #3027

Draft

tomchapman and others added 12 commits December 20, 2024 14:00

Set coordinates on FakeMesh

15cffda

otherwise when Coordinates setters call mesh->communicate() and that calls calcParallelSlices() and that calls getCoordinates() the coordinates field will be a null pointer.

Add Coordinates::communicateDz() and call after constructing Coordinates

77fb0c9

to prevent getUniform(coords->zlength()) failing because the field is not const.

Fix bug in test_snb

09162ca

Update existing dy and J fields, to avoid invalid values in the guard cells.

Apply clang-format changes

834b090

Merge pull request #3056 from boutproject/fix-after-reactoring-commun…

e117fa5

…icate Fixes to address failing tests after changes to refactor-coordinates

Merge remote-tracking branch 'remotes/boutproject/next' into refactor…

4b5bee3

…-coordinates-with-next-merged-in

Fix merge

52b817c

Include changes from commit: "Return `std::optional` from `invert3x3`" SHA-1: c42dc24 which was not merged due to splitting files.

Formatting (whitespace changes only)

fd099bf

Fix for failing fci tests. Need to use Coordinates::DDY() not bout::d…

d52a724

…erivatives::index::DDY() as the former checks canToFromFieldAligned().

Revert "Use free-function derivatives in metric tensor classes"

f7706f7

This reverts commit e5010e4. Fix for failing fci tests.

Apply black changes

58b66f6

ZedThree mentioned this pull request Mar 5, 2025

Time for new release: 5.2 #3084

Open

dschwoerer mentioned this pull request Mar 17, 2026

Add cell volume and areas #3326

Open

1 task

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

Conversation

tomc271 commented Feb 23, 2024

Uh oh!

Uh oh!

Uh oh!

bendudson commented Feb 23, 2024

Uh oh!

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

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

tomc271 commented Sep 30, 2025

Uh oh!

dschwoerer commented Mar 20, 2026

Uh oh!

tomc271 commented Mar 20, 2026

Uh oh!

dschwoerer commented Mar 20, 2026

Uh oh!

dschwoerer commented Mar 20, 2026

Uh oh!

dschwoerer commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants