Simplify Coordinates in 3d metrics branch #2179
Conversation
[skip travis] (just docs + whitespace) Fixes #2129
Think this happened in a merge?
Triggers lots of clang-tidy warnings
MZ no longer has to be a power of two plus one Co-authored-by: Ben Dudson <bd512@york.ac.uk>
ParallelTransform::toFieldAligned(const Field2D&) and ParallelTransform::fromFieldAligned(const Field2D&) do nothing (since Field2D are axisymmetric quantities) but simplify treating Coordinates::FieldMetric quantities (which might be Field2D or Field3D depending on configure options) in the same way.
All derivatives used while creating Coordinates objects are in the geometry() method. There was a problem that standard versions of the derivatives need a Coordinates pointer in the Fields passed in. By moving the call to geometry() out of the constructor, we have removed the need for workarounds to take derivatives before there is a valid Coordinates pointer.
Field2Ds are axisymmetric and don't need to shift to or from field-aligned. With the recent change removing the special toFieldAligned override in field2d.hxx, instead relying on the templates in field.hxx and the ParallelTransform::toFieldAligned(Field2D) and ParallelTransform::fromFieldAligned(Field2D) methods, interp_to would fail on a Field2D when constructing a ShiftedMetric transform by interpolating zShift. Skipping the to/from field-aligned transformations entirely for Field2D avoids this.
When using 3d metrics it is not possible to construct a ShiftedMetric transform by interpolating zShift to CELL_YLOW, because the interpolation would require a shift from field-aligned coordinates using the ShiftedMetric which is being constructed (since zShift is not axisymmetric when 3d metrics are allowed). Now throw an exception in this case, and suggest creating zShift_CELL_YLOW in the grid file as a solution.
…ixes Upgrade all examples to use PhysicsModel
CMake: Set PYTHONPATH for tests
Add back some missing Fields documentation (and clang-format)
Ensures we don't get a nullptr from field->getCoordinates().
Requires the Laplace_perpXY operator, which is not implemented for 3d metrics.
Default preconditioner uses the 'cyclic' Laplace solver, which is not implemented for 3d metrics.
Transformation to field-aligned coordinates in interpolateAndExtrapolate(Field3D) gets rid of guard and boundary cells. Skip doing this if we don't need the interpolation. Allows the case where Coordinates are initialised from input file expressions (where extrapolation is disabled) to work more often, since we keep valid boundary cell values.
It is useful to be able to request a Field at a certain location from Mesh::get() - when the grid data comes from input file expressions, these can now be evaluated at the correct location. Note: includes a fix for GridFromOptionsTest::CoordinatesYlowInterp so it now actually interpolates to CELL_YLOW, not CELL_XLOW.
…nates Conflicts: include/bout/mesh.hxx include/field.hxx src/mesh/coordinates.cxx src/mesh/mesh.cxx tests/MMS/diffusion2/diffusion.cxx tests/MMS/spatial/diffusion/diffusion.cxx tests/unit/mesh/data/test_gridfromoptions.cxx
Preconditioner uses the 'cyclic' Laplace solver, which is not implemented for 3d metrics.
Give _ylow variables in [mesh] section of BOUT.inp, so test-twistshift-staggered can read them and does not need to interpolate. This is necessary when using 3d metrics.
|
This PR now includes a merge of recent updates in The main commits to review are just 11ba037, 91174cc, 5a77878 and 6a9e00b. 9e58e8a and cd0500f add some checking to 8d30f3c, a84789f, 865d274, c0fda06, 98cbe71 and ff6b801 were fixing or skipping tests (not all directly related to this PR). |
johnomotani
force-pushed
the
coord3d_merged2_simplify_Coordinates
branch
from
224c7c6
to
7b52468
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (diffusion_par > 0.0) { | ||
| // xqx addition, begin | ||
| // Use Spitzer thermal conductivities | ||
| BoutReal Te_tmp1, Ti_tmp1; |
There was a problem hiding this comment.
warning: variable Te_tmp1 is not initialized [cppcoreguidelines-init-variables]
BoutReal Te_tmp1, Ti_tmp1;
^
= NAN| if (diffusion_par > 0.0) { | ||
| // xqx addition, begin | ||
| // Use Spitzer thermal conductivities | ||
| BoutReal Te_tmp1, Ti_tmp1; |
There was a problem hiding this comment.
warning: variable Ti_tmp1 is not initialized [cppcoreguidelines-init-variables]
BoutReal Te_tmp1, Ti_tmp1;
^
= NAN| // Smooth j in x | ||
| if (smooth_j_x) | ||
| Jpar = smooth_x(Jpar); | ||
| Field3D kappa_par_i_fl, kappa_par_e_fl; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
Field3D kappa_par_i_fl, kappa_par_e_fl;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| #include <initialprofiles.hxx> | ||
|
|
||
| int reaction(BoutReal time); | ||
| class Split_operator : public PhysicsModel { |
There was a problem hiding this comment.
warning: constructor does not initialize these fields: rate [cppcoreguidelines-pro-type-member-init]
class Split_operator : public PhysicsModel {
^|
|
||
| protected: | ||
| int init(bool restarting) override; | ||
| int rhs(BoutReal) override; |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
int rhs(BoutReal) override;
^
/*UNUSED_t*/|
|
||
| // Tell BOUT++ to solve N | ||
| SOLVE_FOR(N); | ||
| if (Dx > 0.0) |
There was a problem hiding this comment.
warning: statement should be inside braces [readability-braces-around-statements]
if (Dx > 0.0)
^
{|
|
||
| return 0; | ||
| } | ||
| if (Dy > 0.0) |
There was a problem hiding this comment.
warning: statement should be inside braces [readability-braces-around-statements]
if (Dy > 0.0)
^
{|
|
||
| int physics_run(BoutReal UNUSED(t)) { | ||
| mesh->communicate(N); // Communicate guard cells | ||
| if (Dz > 0.0) |
There was a problem hiding this comment.
warning: statement should be inside braces [readability-braces-around-statements]
if (Dz > 0.0)
^
{| apar_solver = Laplacian::create(globalOptions->getSection("aparsolver")); | ||
| } | ||
| // just define a macro for V_E dot Grad | ||
| #define vE_Grad(f, p) (b0xGrad_dot_Grad(p, f) / coord->Bxy) |
There was a problem hiding this comment.
warning: function-like macro vE_Grad used; consider a constexpr template function [cppcoreguidelines-macro-usage]
#define vE_Grad(f, p) (b0xGrad_dot_Grad(p, f) / coord->Bxy)
^| // just define a macro for V_E dot Grad | ||
| #define vE_Grad(f, p) (b0xGrad_dot_Grad(p, f) / coord->Bxy) | ||
|
|
||
| class TwoFluid : public PhysicsModel { |
There was a problem hiding this comment.
warning: constructor does not initialize these fields: Te_x, Ti_x, Ni_x, Vi_x, bmag, rho_s, fmei, AA, ZZ, lambda_ei, lambda_ii, nu_hat, mui_hat, wci, nueix, nuiix, beta_p, estatic, ZeroElMass, zeff, nu_perp, evolve_rho, evolve_te, evolve_ni, evolve_ajpar, evolve_vi, evolve_ti, ShearFactor, coord, maybe_ylow [cppcoreguidelines-pro-type-member-init]
class TwoFluid : public PhysicsModel {
^
|
The test failures are just timeouts (which should be fixed if we merged next again now #2197's gone in), so I think this is ready to go. I'll leave merging to the 3d metrics team 😃 |
The implementations were removed in #2179, missed the declarations
Removes call to
Coordinates::geometry()from theCoordinatesconstructor. This results in a 2-step initialisation ofCoordinatesand means that at the places where we have to calculate derivatives, theFields have a validCoordinatespointer. We can then remove some work-arounds for not-existing Coordinates.