3D metrics #2025

dschwoerer · 2020-04-29T12:29:25Z

(copied from comment below for better visibility)

I tried to look at all open comments, and I think this is the list of stuff that needs to be done (feel free to edit)

Unit tests?
@ZedThree 3D metrics #2025 (comment)
- Add unit test for LaplaceXZPetsc (3D metrics) #2207

I've spotted a couple of places that could have useful unit tests, so I will have a stab at implementing those.

Some question for @bshanahan : 3D metrics #2025 (comment)
Fix 3D metrics #2025 (comment)
Fix 3D metrics #2025 (comment)
Needs answering 3D metrics #2025 (comment)
Commit f5dccc9 by @bshanahan
Cleanup: 3D metrics #2025 (comment)
cmake issues 3D metrics #2025 (review) @ZedThree wanted to push
fix CI errors
Do we need documentation?
3D metrics #2025 (comment)

Allows to compile and run all tests by running: make -j 4 build-check-all && make check-all

The shift-transform in staggered location requieres dz. To calculate the staggered dz in y direction, the parallel transform is requiered. Thus we special handel the case if dz is constant, and we can do a trivial staggering, without a parallel transform. For non-const dz, staggered dz needs to be given in an input file, so we don't have to interpolate. Also make isConst a free function, similar to min and max.

If we are deeper nested, omp_get_thread_num isn't unique anymore

* Add metric3D to cmake * cmake with -DENABLE_METRIC_3D=ON to get 3D metrics * Due to the out-of-source build we need to python dirs, one for noarch files, and one for arched ones

otherwise python finds boututils in one pythonpath, and does not look in other locations for the other file.

Some solver throw errors otherwise

Read the environment variable BOUT_SHOW_BACKTRACE. If the variable is set, show the backtrace, otherwise just show the message. The allows to get the backtrace without having to attache gdb, or catch the exception. The default behaviour is not changed.

github-actions

clang-tidy made some suggestions

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

include/bout/coordinates.hxx

Previously no boundary was detection because the index cannot be out of bounds in both directions

github-actions

clang-tidy made some suggestions

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

include/bout/coordinates.hxx

include/bout/paralleltransform.hxx

github-actions · 2021-03-22T13:36:26Z

include/derivs.hxx

-    REGION region = RGN_NOBNDRY) { \
-  return func(f, outloc, toString(method), toString(region)); \
-}
+#define DERIV_FUNC_REGION_ENUM_TO_STRING(func, T, Tr)                                  \


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

#define DERIV_FUNC_REGION_ENUM_TO_STRING(func, T, Tr) \ ^

github-actions · 2021-03-22T13:36:26Z

src/field/vector2d.cxx

@@ -94,9 +94,9 @@ void Vector2D::toCovariant() {
      const auto metric = localmesh->getCoordinates(location);

      // Need to use temporary arrays to store result
-      Field2D gx{emptyFrom(x)}, gy{emptyFrom(y)}, gz{emptyFrom(z)};
+      Coordinates::FieldMetric gx{emptyFrom(x)}, gy{emptyFrom(y)}, gz{emptyFrom(z)};


warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Coordinates::FieldMetric gx{emptyFrom(x)}, gy{emptyFrom(y)}, gz{emptyFrom(z)}; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

github-actions · 2021-03-22T13:36:26Z

src/field/vector2d.cxx

@@ -146,9 +146,9 @@ void Vector2D::toContravariant() {
      const auto metric = localmesh->getCoordinates(location);

      // Need to use temporary arrays to store result
-      Field2D gx{emptyFrom(x)}, gy{emptyFrom(y)}, gz{emptyFrom(z)};
+      Coordinates::FieldMetric gx{emptyFrom(x)}, gy{emptyFrom(y)}, gz{emptyFrom(z)};


warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Coordinates::FieldMetric gx{emptyFrom(x)}, gy{emptyFrom(y)}, gz{emptyFrom(z)}; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Returning non-const objects has no disadvantage for const correctness and is easier to read.

* Previously, only the inner-most boundary point was set, which could cause problems with interpolation

github-actions

clang-tidy made some suggestions

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

github-actions · 2021-03-26T11:12:02Z

src/field/vector2d.cxx

@@ -371,11 +371,11 @@ const Vector3D Vector2D::operator/(const Field3D &rhs) const {

 ////////////////// DOT PRODUCT ///////////////////

-const Field2D Vector2D::operator*(const Vector2D &rhs) const {
+const Coordinates::FieldMetric Vector2D::operator*(const Vector2D& rhs) const {


warning: return type const Coordinates::FieldMetric (aka const Field2D) is const-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]

const Coordinates::FieldMetric Vector2D::operator*(const Vector2D& rhs) const { ^~~~~~

github-actions · 2021-03-26T11:12:02Z

src/field/vector2d.cxx

@@ -474,7 +474,7 @@ const Vector3D operator*(const Field3D &lhs, const Vector2D &rhs) {
 ***************************************************************/

 // Return the magnitude of a vector
-const Field2D abs(const Vector2D &v, const std::string& region) {
+const Coordinates::FieldMetric abs(const Vector2D& v, const std::string& region) {


warning: return type const Coordinates::FieldMetric (aka const Field2D) is const-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]

const Coordinates::FieldMetric abs(const Vector2D& v, const std::string& region) { ^~~~~~

github-actions · 2021-03-26T11:12:02Z

src/invert/laplace/impls/serial_band/serial_band.cxx

-    BoutReal coef1=0.0, coef2=0.0, coef3=0.0, coef4=0.0, 
-      coef5=0.0, coef6=0.0, kwave;
+
+    BoutReal coef1 = 0.0, coef2 = 0.0, coef3 = 0.0, coef4 = 0.0, coef5 = 0.0, coef6 = 0.0;


warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

BoutReal coef1 = 0.0, coef2 = 0.0, coef3 = 0.0, coef4 = 0.0, coef5 = 0.0, coef6 = 0.0; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/invert/laplacexz/impls/petsc/laplacexz-petsc.cxx

src/mesh/boundary_standard.cxx

Return non-const should be preferred

…erged2 Conflicts: include/bout/invert/laplacexz.hxx src/invert/laplacexz/impls/petsc/laplacexz-petsc.cxx

After communication we might have invalid points, this is fixed by applying boundaries.

Merge recent changes to fix CI

[3D metric] Use Bxy.ydown() for div_a_laplace_perp

[3D metric] Ensure boundaries are valid

ZedThree · 2021-07-01T12:29:26Z

Once #2357 has been merged, I think this is ready to go. We know there are issues with LaplaceXZ, but at least some of them exist in next (see #2207, #2359)

I do think we could do with more 3D-specific tests, but there is a decent amount of coverage already.

* next: Mark unused function arguments with MAYBE_UNUSED De-abbreviate exception message Use constants in setting default_dz for Coordinates unit test Fix expectation for default value Do not ignore default value Fix mpark_submodule if not called from local directory Add additional targets for submodules Some basic tests of Coordinates inputs Basic tests of Coordinates Enable FakeGridDataSource to provide values to Mesh

github-actions

clang-tidy made some suggestions

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

github-actions · 2021-07-01T16:49:35Z

include/bout/paralleltransform.hxx

@@ -284,14 +304,13 @@ private:
   * Shift a 2D field in Z.
   * Since 2D fields are constant in Z, this has no effect
   */
-  const Field2D shiftZ(const Field2D& f, const Field2D& UNUSED(zangle),
-                       const std::string UNUSED(region) = "RGN_NOX") const {
+  Field2D shiftZ(const Field2D& f, const Field2D& UNUSED(zangle),


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

Field2D shiftZ(const Field2D& f, const Field2D& UNUSED(zangle), ^ static

github-actions · 2021-07-01T16:49:35Z

include/bout/paralleltransform.hxx

-  const Field2D shiftZ(const Field2D& f, const Field2D& UNUSED(zangle),
-                       const std::string UNUSED(region) = "RGN_NOX") const {
+  Field2D shiftZ(const Field2D& f, const Field2D& UNUSED(zangle),
+                 const std::string UNUSED(region) = "RGN_NOX") const {


warning: the const qualified parameter UNUSED_region is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]

const std::string UNUSED(region) = "RGN_NOX") const { ^ & /github/workspace/include/unused.hxx:29:20: note: expanded from macro 'UNUSED' # define UNUSED(x) UNUSED_ ## x __attribute__((unused)) ^ note: expanded from here

github-actions · 2021-07-01T16:49:35Z

include/bout/paralleltransform.hxx

-  const Field2D shiftZ(const Field2D& f, const Field2D& UNUSED(zangle),
-                       REGION UNUSED(region)) const {
+               "const std::string& region = \"RGN_NOX\") instead")]] Field2D
+  shiftZ(const Field2D& f, const Field2D& UNUSED(zangle), REGION UNUSED(region)) const {


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

shiftZ(const Field2D& f, const Field2D& UNUSED(zangle), REGION UNUSED(region)) const { ^ ~~~~~~

github-actions · 2021-07-01T16:49:35Z

include/bout/paralleltransform.hxx

-                         const YDirectionType y_direction_out,
-                         const std::string& region = "RGN_NOX") const;
+  Field3D shiftZ(const Field3D& f, const Tensor<dcomplex>& phs,
+                 const YDirectionType y_direction_out,


warning: parameter y_direction_out is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

const YDirectionType y_direction_out, ^~~~~~

github-actions · 2021-07-01T16:49:35Z

include/bout/paralleltransform.hxx

+                 const YDirectionType y_direction_out,
+                 const std::string& region = "RGN_NOX") const;
+  FieldPerp shiftZ(const FieldPerp& f, const Tensor<dcomplex>& phs,
+                   const YDirectionType y_direction_out,


warning: parameter y_direction_out is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

const YDirectionType y_direction_out, ^~~~~~

github-actions · 2021-07-01T16:49:38Z

src/mesh/boundary_standard.cxx

-void BoundaryFree::apply_ddt(Field2D& UNUSED(f)) {
-  // Do nothing for free boundary
-}
+    int jx, jy, jz, jzp, jzm;


warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

int jx, jy, jz, jzp, jzm; ^~~~~~~~~~~~~~~~~~~~~~~~~

github-actions · 2021-07-01T16:49:38Z

src/mesh/boundary_standard.cxx

-void BoundaryFree::apply_ddt(Field2D& UNUSED(f)) {
-  // Do nothing for free boundary
-}
+    int jx, jy, jz, jzp, jzm;


warning: variable jx is not initialized [cppcoreguidelines-init-variables]

int jx, jy, jz, jzp, jzm; ^ = 0

github-actions · 2021-07-01T16:49:38Z

src/mesh/boundary_standard.cxx

-void BoundaryFree::apply_ddt(Field2D& UNUSED(f)) {
-  // Do nothing for free boundary
-}
+    int jx, jy, jz, jzp, jzm;


warning: variable jy is not initialized [cppcoreguidelines-init-variables]

int jx, jy, jz, jzp, jzm; ^ = 0

github-actions · 2021-07-01T16:49:38Z

src/mesh/boundary_standard.cxx

-void BoundaryFree::apply_ddt(Field2D& UNUSED(f)) {
-  // Do nothing for free boundary
-}
+    int jx, jy, jz, jzp, jzm;


warning: variable jz is not initialized [cppcoreguidelines-init-variables]

int jx, jy, jz, jzp, jzm; ^ = 0

github-actions · 2021-07-01T16:49:39Z

src/mesh/boundary_standard.cxx

-void BoundaryFree::apply_ddt(Field2D& UNUSED(f)) {
-  // Do nothing for free boundary
-}
+    int jx, jy, jz, jzp, jzm;


warning: variable jzp is not initialized [cppcoreguidelines-init-variables]

int jx, jy, jz, jzp, jzm; ^ = 0

ZedThree · 2021-07-01T16:53:32Z

@bshanahan @dschwoerer I'm happy to merge this when the tests pass, but between you please could you create one or two integrated 3D tests? Analytic answers would obviously be lovely, but a regression test would be fine! Maybe two waves in a rotating ellipsis?

bshanahan · 2021-07-07T12:32:05Z

🥳 🍾

dschwoerer and others added 30 commits March 17, 2020 18:04

Do not make fields non-const

06d6cfb

Set d1_dz

236fd8e

Fix most tests

0dab643

update examples for non-const dz

571b1ab

Update boutcore for non-const dz

02b72e0

fix examples

b78177f

fix bracket for 3D

7f524f8

Fix tests for 3d metrics

766c6b0

Add more -all targets for checks

2cf2f25

Allows to compile and run all tests by running: make -j 4 build-check-all && make check-all

Ensure ffts are OpenMP thread safe

151e8e2

If we are deeper nested, omp_get_thread_num isn't unique anymore

Re-enable fixed test

07b48ee

Add bout_config to cmake

4d1b5a7

* Add metric3D to cmake * cmake with -DENABLE_METRIC_3D=ON to get 3D metrics * Due to the out-of-source build we need to python dirs, one for noarch files, and one for arched ones

Move boutconfig to own folder.

109b906

otherwise python finds boututils in one pythonpath, and does not look in other locations for the other file.

update configure

f739390

Update operators for FCI compatability

451037f

Merge remote-tracking branch 'boutproject/master' into coord3d_merged2

31de678

Run 3D metric test with openmp enabled

5d40a88

Merge remote-tracking branch 'boutproject/next' into coord3d_merged2

1cdfa35

Fixes for dz being a Field

4ca152f

Adjust field compat messages for fmt

9d38bf0

Evolve something in test-communications

1a4cd36

Some solver throw errors otherwise

Conditionally show backtrace

d19ba26

Read the environment variable BOUT_SHOW_BACKTRACE. If the variable is set, show the backtrace, otherwise just show the message. The allows to get the backtrace without having to attache gdb, or catch the exception. The default behaviour is not changed.

Don't use old-style-cast

0a5a6d6

Don't use old-style-cast in boutexception

42c525d

Better error if field is not positive

a7705fe

reduce code duplications in test-communications

9060707

Merge remote-tracking branch 'boutproject/next' into coord3d_merged2

cd03090

Ensure we communicate in both directions

4b7611c

Fix test-griddata for 3d metrics

2d52477

github-actions bot reviewed Mar 3, 2021

View reviewed changes

Fix boundary detection

2ef34fd

Previously no boundary was detection because the index cannot be out of bounds in both directions

github-actions bot reviewed Mar 22, 2021

View reviewed changes

dschwoerer and others added 2 commits March 23, 2021 09:37

Return non-const should be preferred

6ab2136

Returning non-const objects has no disadvantage for const correctness and is easier to read.

Set all boundary cells in petsc-laplacexz.

9eae08e

* Previously, only the inner-most boundary point was set, which could cause problems with interpolation

github-actions bot reviewed Mar 26, 2021

View reviewed changes

ZedThree and others added 14 commits April 9, 2021 10:23

Merge pull request #2269 from boutproject/less-const

404f222

Return non-const should be preferred

Merge branch 'next' of github.com:boutproject/BOUT-dev into coord3d_m…

3a63092

…erged2 Conflicts: include/bout/invert/laplacexz.hxx src/invert/laplacexz/impls/petsc/laplacexz-petsc.cxx

Merge remote-tracking branch 'origin/next' into coord3d_merged2

5801465

Use Bxy.ydown() for div_a_laplace_perp

a351b17

Ensure boundaries are valid

af698ef

After communication we might have invalid points, this is fixed by applying boundaries.

Merge remote-tracking branch 'origin/next' into coords-boundary

c03c91e

Merge recent changes to fix CI

Merge pull request #2312 from boutproject/3d_div_a_laplace_perp_fix

ffabb13

[3D metric] Use Bxy.ydown() for div_a_laplace_perp

fix bad merge

7701426

Merge remote-tracking branch 'origin/next' into coords-boundary

5edd3e1

Fix pcr for dz being a Field2D

b5712fb

Don't specify boundary condition

d54512f

Merge remote-tracking branch 'origin/next' into coords-boundary

58843dd

Temporarily disable python interface with 3D in CI

867e303

Merge pull request #2326 from boutproject/coords-boundary

2d3635f

[3D metric] Ensure boundaries are valid

dschwoerer mentioned this pull request Jul 1, 2021

Add unit test for LaplaceXZPetsc (3D metrics) #2207

Closed

CMake: Add CMakeLists.txt for test-laplacexz

1ba36af

github-actions bot reviewed Jul 1, 2021

View reviewed changes

ZedThree approved these changes Jul 6, 2021

View reviewed changes

ZedThree merged commit 4b75196 into next Jul 7, 2021

ZedThree deleted the coord3d_merged2 branch November 30, 2021 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3D metrics #2025

3D metrics #2025

dschwoerer commented Apr 29, 2020 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot Mar 22, 2021

github-actions bot Mar 22, 2021

github-actions bot Mar 22, 2021

github-actions bot left a comment

github-actions bot Mar 26, 2021

github-actions bot Mar 26, 2021

github-actions bot Mar 26, 2021

ZedThree commented Jul 1, 2021

github-actions bot left a comment

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

github-actions bot Jul 1, 2021

ZedThree commented Jul 1, 2021

bshanahan commented Jul 7, 2021

3D metrics #2025

3D metrics #2025

Conversation

dschwoerer commented Apr 29, 2020 • edited Loading

github-actions bot 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 Mar 22, 2021

Choose a reason for hiding this comment

github-actions bot Mar 22, 2021

Choose a reason for hiding this comment

github-actions bot Mar 22, 2021

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Mar 26, 2021

Choose a reason for hiding this comment

github-actions bot Mar 26, 2021

Choose a reason for hiding this comment

github-actions bot Mar 26, 2021

Choose a reason for hiding this comment

ZedThree commented Jul 1, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

github-actions bot Jul 1, 2021

Choose a reason for hiding this comment

ZedThree commented Jul 1, 2021

bshanahan commented Jul 7, 2021

dschwoerer commented Apr 29, 2020 •

edited

Loading