Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work on elm-pb for RAJA #2414

Merged
merged 14 commits into from
Sep 2, 2021
Merged

Conversation

bendudson
Copy link
Contributor

@bendudson bendudson commented Sep 1, 2021

Split elm-pb into standard and outerloop examples:

The elm-pb example is essentially the same as in next, apart from some minor formatting changes and fixes. It uses the standard BOUT++ field operations.

The elm-pb-outerloop example uses outer loops. The current code doesn't compile (uses Yining's old operators), but will now be modified / rewritten.

This includes the fix to ShiftedMetricInterp, cherry picked from #2398

Output from elm-pb (default input) on one core:

1.000e+00         44       3.36e+01    97.4    1.2    0.0    0.1    1.2
2.000e+00         37       2.76e+01    97.5    1.2    0.0    0.1    1.1
3.000e+00         37       2.79e+01    97.4    1.2    0.0    0.1    1.2
4.000e+00         37       2.74e+01    97.4    1.2    0.0    0.1    1.3

Output from elm-pb-outerloop (default input) on one core:

1.000e+00         44       2.07e+01    95.7    2.1    0.1    0.2    2.0
2.000e+00         37       1.69e+01    95.6    2.1    0.1    0.2    1.9
3.000e+00         37       1.73e+01    95.6    2.0    0.1    0.3    2.0
4.000e+00         37       1.65e+01    95.7    2.1    0.1    0.2    2.0

Detailed output not yet compared.
Only compiled & run on CPU, not yet with GPU / RAJA / Cori complications.

The `elm-pb` example is essentially the same as in `next`, apart from
some minor formatting changes and fixes. It uses the standard BOUT++
field operations.

The `elm-pb-outerloop` example uses outer loops. The current code
doesn't compile (uses Yining's old operators), but will now be modified.
Drive-by fix suggested by clang-tidy-review.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

CELL_LOC loc = CELL_CENTRE;

class ELMpb : public PhysicsModel {
private:
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: constructor does not initialize these fields: constn0, n0_height, n0_ave, n0_width, n0_center, n0_bottom_x, Nbar, Tibar, Tebar, Tconst, n0_fake_prof, T0_fake_prof, density, Bbar, Lbar, Tbar, Va, dnorm, dia_fact, delta_i, omega_i, diffusion_p4, diffusion_u4, diffusion_a4, diffusion_par, heating_P, hp_width, hp_length, sink_P, sp_width, sp_length, sink_Ul, su_widthl, su_lengthl, sink_Ur, su_widthr, su_lengthr, viscos_par, viscos_perp, hyperviscos, Upara2, include_curvature, include_jpar0, compress, evolve_pressure, gyroviscous, vacuum_pressure, vacuum_trans, nonlinear, evolve_jpar, g, phi_curv, bm_exb, bm_mag, bm_exb_flag, bm_mag_flag, diamag, diamag_grad_t, diamag_phi0, eHall, AA, D_0, D_s, x0, sign, Psiaxis, Psibndry, withflow, K_H_term, D_min, experiment_Er, nogradparj, filter_z, filter_z_mode, low_pass_z, zonal_flow, zonal_field, zonal_bkgd, split_n0, relax_j_vac, relax_j_tconst, smooth_j_x, jpar_bndry_width, sheath_boundaries, parallel_lr_diff, phi_constraint, include_rmp, simple_rmp, rmp_factor, rmp_ramp, rmp_freq, rmp_rotate, rmp_vac_mask, vac_lund, core_lund, vac_resist, core_resist, spitzer_resist, Zeff, hyperresist, ehyperviscos, damp_width, damp_t_const [cppcoreguidelines-pro-type-member-init]

class ELMpb : public PhysicsModel {
      ^

std::unique_ptr<Laplacian> aparSolver{nullptr};

const Field2D N0tanh(BoutReal n0_height, BoutReal n0_ave, BoutReal n0_width,
BoutReal n0_center, BoutReal n0_bottom_x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const Field2D N0tanh(BoutReal n0_height, BoutReal n0_ave, BoutReal n0_width,
^~~~~~

result.allocate();

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
BoutReal Jysep;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

result.allocate();

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
BoutReal Jysep;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
         ^
                 = NAN

result.allocate();

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
BoutReal Jysep;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
                  ^
                               = NAN

N0 = P0 / (Ti0 + Te0);
} else {
if (mesh->get(N0, "Niexp")) { // N_i0
throw BoutException("Error: Cannot read Ni0 from grid\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion int -> bool [readability-implicit-bool-conversion]

if (mesh->get(N0, "Niexp")) { // N_i0
    ^
                           != 0

}

if (mesh->get(Ti0, "Tiexp")) { // T_i0
throw BoutException("Error: Cannot read Ti0 from grid\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion int -> bool [readability-implicit-bool-conversion]

if (mesh->get(Ti0, "Tiexp")) { // T_i0
    ^
                            != 0

}

if (mesh->get(Te0, "Teexp")) { // T_e0
throw BoutException("Error: Cannot read Te0 from grid\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion int -> bool [readability-implicit-bool-conversion]

if (mesh->get(Te0, "Teexp")) { // T_e0
    ^
                            != 0


// Parallel gradient along perturbed field-line
const Field3D Grad_parP(const Field3D& f, CELL_LOC loc = CELL_DEFAULT) {

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: return type const Field3D is const-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]

const Field3D Grad_parP(const Field3D& f, CELL_LOC loc = CELL_DEFAULT) {
^~~~~~

}

bool first_run = true; // For printing out some diagnostics first time around

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 first_run has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

bool first_run = true; // For printing out some diagnostics first time around
     ^

To minimise changes between elm-pb and elm-pb-outerloop
Merging operations into a single large loop, which can then
be iterated over using RAJA or BOUT_FOR.

To enable operations to be merged, run-time switches have to be
converted to compile-time:

- Compile-time flags (e.g. NONLINEAR) are set to `true` or `false` at
  the top of the `elm-pb-outerloop.cxx` file.
- At run-time the options are read in the same way as `elm-pb`
- The run-time value is compared against the compile-time value
  (e.g. input `nonlinear` and compile flag `NONLINEAR`). If they
  are different then an exception is thrown. This is so that the
  simulation always uses the value expected from the input file.
  For convenience a `CHECK_SETTING` macro is defined.
- To turn on and off terms in the equations, an `EVAL_IF` macro
  is defined to improve readability.

Terms which have been moved to single index operators are left in
the code for now, but commented out. This is to allow comparison
of results while testing.
Extracts the index and passes on as int
Removing unused settings in data/BOUT.inp

Ensuring that compile flags are consistent with input settings in
`elm-pb-outerloop`.

Compiles without RAJA, takes same number of iterations for `elm-pb`
and `elm-pb-outerloop`. Haven't checked output yet.
Need to use another couple of layers of macro expansion
to include the __LINE__ as an actual line number.
__FILE__ evaluates to a string, so doesn't concatenate to give
a valid symbol.
Using BOUT_OVERRIDE_DEFAULT_OPTION, set sensible default
boundary conditions for potential phi.
If overrideDefault is used to set a new default, and then
the value isn't used, an exception should not be raised.
Previously assumed zlength == 2pi when converting between
zShift (radians) and Z grid cell number.

Now ShiftedMetricInterp takes the same constructor arguments as
ShiftedMetric.

For the elm-pb standard test, after one output step the vorticity at
the branch cut is within 0.1% for shiftedinterp & shifted. The same
number of RHS calls is also taken.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

*******************************************************************************/

#define RUN_WITH_RAJA false // Use RAJA loops?

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro RUN_WITH_RAJA used to declare a constant; consider using a constexpr constant [cppcoreguidelines-macro-usage]

#define RUN_WITH_RAJA false   // Use RAJA loops?
        ^

#define RUN_WITH_RAJA false // Use RAJA loops?

#define EVOLVE_JPAR false // Evolve ddt(Jpar) rather than ddt(Psi)?
#define RELAX_J_VAC false // Relax to zero-current in the vacuum?
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro EVOLVE_JPAR used to declare a constant; consider using a constexpr constant [cppcoreguidelines-macro-usage]

#define EVOLVE_JPAR false     // Evolve ddt(Jpar) rather than ddt(Psi)?
        ^


#define EVOLVE_JPAR false // Evolve ddt(Jpar) rather than ddt(Psi)?
#define RELAX_J_VAC false // Relax to zero-current in the vacuum?
#define EHALL false // Include electron pressure effects in Ohm's law?
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro RELAX_J_VAC used to declare a constant; consider using a constexpr constant [cppcoreguidelines-macro-usage]

#define RELAX_J_VAC false     // Relax to zero-current in the vacuum?
        ^

#define EVOLVE_JPAR false // Evolve ddt(Jpar) rather than ddt(Psi)?
#define RELAX_J_VAC false // Relax to zero-current in the vacuum?
#define EHALL false // Include electron pressure effects in Ohm's law?
#define DIAMAG_PHI0 true // Balance ExB against Vd for stationary equilibrium?
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro EHALL used to declare a constant; consider using a constexpr constant [cppcoreguidelines-macro-usage]

#define EHALL false           // Include electron pressure effects in Ohm's law?
        ^

#define RELAX_J_VAC false // Relax to zero-current in the vacuum?
#define EHALL false // Include electron pressure effects in Ohm's law?
#define DIAMAG_PHI0 true // Balance ExB against Vd for stationary equilibrium?
#define DIAMAG_GRAD_T false // Include Grad_par(Te) term in Psi equation?
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro DIAMAG_PHI0 used to declare a constant; consider using a constexpr constant [cppcoreguidelines-macro-usage]

#define DIAMAG_PHI0 true      // Balance ExB against Vd for stationary equilibrium?
        ^

/// Gradient along perturbed magnetic field
/// Note: This relies on Psi_acc, B0_acc, i, i2d and rmp_Psi_acc
#define GRAD_PARP(f_acc) \
(Grad_par(f_acc, i) \
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-like macro GRAD_PARP used; consider a constexpr template function [cppcoreguidelines-macro-usage]

#define GRAD_PARP(f_acc)                                                \
        ^

pointer(options)->get(#var3, var3, def); \
pointer(options)->get(#var4, var4, def); \
pointer(options)->get(#var5, var5, def); \
#define OPTION6(options, var1, var2, var3, var4, var5, var6, def){ \
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-like macro OPTION6 used; consider a constexpr template function [cppcoreguidelines-macro-usage]

#define OPTION6(options, var1, var2, var3, var4, var5, var6, def){      \
        ^

namespace { \
const auto user_default##__FILE__##__LINE__ = \
Options::root()[name].overrideDefault(value); } \
#define BOUT_OVERRIDE_DEFAULT_OPTION(name, value) \
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-like macro BOUT_OVERRIDE_DEFAULT_OPTION used; consider a constexpr template function [cppcoreguidelines-macro-usage]

#define BOUT_OVERRIDE_DEFAULT_OPTION(name, value)               \
        ^

#ifndef BOUT_CONCAT
/// Utility to evaluate and concatenate macro symbols
/// Note that ## operator doesn't evaluate symols A or B
#define BOUT_CONCAT_(A,B) A##B
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-like macro BOUT_CONCAT_ used; consider a constexpr template function [cppcoreguidelines-macro-usage]

#define BOUT_CONCAT_(A,B) A##B
        ^

/// Utility to evaluate and concatenate macro symbols
/// Note that ## operator doesn't evaluate symols A or B
#define BOUT_CONCAT_(A,B) A##B
#define BOUT_CONCAT(A,B) BOUT_CONCAT_(A,B)
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-like macro BOUT_CONCAT used; consider a constexpr template function [cppcoreguidelines-macro-usage]

#define BOUT_CONCAT(A,B) BOUT_CONCAT_(A,B)
        ^

If python modules are loaded then configure picks up
anaconda libraries rather than system libraries. This seems
to be related to NetCDF.
Build even if BOUT_BUILD_EXAMPLES is off, for testing of
outerloop / RAJA codes.
Configures BOUT++ without Hypre, since that part of the
build is currently failing with linker errors.
@bendudson bendudson merged commit f49e981 into next-hypre-outerloop-cuda-merged Sep 2, 2021
@bendudson bendudson deleted the elm-pb-outerloop branch September 2, 2021 17:07
Merge changes to Bash .sh scripts into Csh .csh scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant