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

Upgrade all examples to use PhysicsModel #2164

Merged
merged 18 commits into from
Dec 14, 2020

Conversation

ZedThree
Copy link
Member

@ZedThree ZedThree commented Dec 2, 2020

Adds a tool to convert the legacy API of physics_init/physics_run to use PhysicsModel instead, and converts all the examples/tests to use PhysicsModel.

This is ahead of removing the old API which will simplify some bits of Solver.

bin/bout-v5-physics-model-upgrader.py converts the free functions to out-of-line definitions for the PhysicsModel methods, and then adds the derived class at the top of the file. I've manually cleaned up a bunch of examples, mostly just inlining the method definitions and making global variables members of the model.

@ZedThree ZedThree added this to the BOUT-5.0 milestone Dec 2, 2020
@ZedThree ZedThree force-pushed the remove-legacy-models-with-fixes branch from 7811f3e to cbb2c56 Compare December 2, 2020 15:26
@ZedThree
Copy link
Member Author

ZedThree commented Dec 2, 2020

The first real trial of clang-tidy-review and it breaks due to:

github.GithubException.GithubException: 502 {"message": "Server Error"}

which is not a terrible helpful error message. It might be because it's trying to send a few hundred review comments all at once.

I think we might need to turn some of the clang-tidy warnings off as well, some of them are just not helpful. For example, there is a warning about magic numbers, which seems to apply to everything which isn't a constant variable initialisation:

warning: 2.0 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

ddt(Te) -= 2.0 / 3.0 * Te0 * B0 * Grad_parP(Vepar / B0, CELL_CENTRE);
           ^

or

warning: 0.1 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

OPTION(options, kappa, 0.1);
                       ^

This actually gets triggered twice for each magic number, because it also appears as readability-magic-numbers

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

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


constexpr BoutReal eV_K = 11605.0; // 1eV = 11605K

class Elm_6f : public PhysicsModel {
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: n0_height, n0_ave, n0_width, n0_center, n0_bottom_x, Tconst, laplace_alpha, Tau_ie, q95_input, local_q, q_alpha, n0_fake_prof, T0_fake_prof, Zi, emass, emass_inv, coef_jpar, delta_e, delta_e_inv, gyroAlv, density, Bbar, Lbar, Tbar, Va, Nbar, Tibar, Tebar, dia_fact, diffusion_par, diffusion_perp, diffusion_n4, diffusion_ti4, diffusion_te4, diffusion_v4, diffusion_u4, heating_P, hp_width, hp_length, sink_vp, sp_width, sp_length, sink_Ul, su_widthl, su_lengthl, sink_Ur, su_widthr, su_lengthr, viscos_par, viscos_perp, hyperviscos, Psipara1, Upara0, Upara1, Upara2, Upara3, Nipara1, Tipara1, Tipara2, Tepara1, Tepara2, Tepara3, Tepara4, Vepara, Vipara, Low_limit, gamma_i_BC, gamma_e_BC, Sheath_width, const_cse, include_curvature, include_jpar0, compress0, evolve_pressure, continuity, gyroviscous, diffusion_coef_Hmode0, diffusion_coef_Hmode1, vacuum_pressure, vacuum_trans, nonlinear, evolve_jpar, g, phi_curv, bm_exb, bm_mag, bracket_method_exb, bracket_method_mag, diamag, energy_flux, energy_exch, diamag_phi0, thermal_force, eHall, AA, Vt0, Vp0, experiment_Er, nogradparj, filter_z, filter_z_mode, low_pass_z, zonal_flow, zonal_field, zonal_bkgd, relax_j_vac, relax_j_tconst, smooth_j_x, filter_nl, jpar_bndry_width, parallel_lagrange, parallel_project, phi_constraint, vac_lund, core_lund, vac_resist, core_resist, spitzer_resist, output_transfer, output_ohm, output_flux_par, hyperresist, ehyperviscos, damp_width, damp_t_const, LnLambda [cppcoreguidelines-pro-type-member-init]

class Elm_6f : public PhysicsModel {
      ^

/*
* This function implements d2/dy2 where y is the poloidal coordinate theta
*/
Field3D Grad2_par2new(const Field3D& f) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Field3D Grad2_par2new(const Field3D& f) {
        ^
static

Field2D result;
result.allocate();

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
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
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Field2D result;
result.allocate();

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
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

Field2D result;
result.allocate();

BoutReal Grid_NX, Grid_NXlimit; // the grid number on x, and the
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

if (hyperresist > 0.0) {
output.write(" Hyper-resistivity coefficient: {:e}\n", hyperresist);
dump.add(hyper_eta_x, "hyper_eta_x", 1);
dump.add(hyper_eta_z, "hyper_eta_z", 1);
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]

dump.add(hyper_eta_z, "hyper_eta_z", 1);
                                     ^
                                     true

return 1;
if (hyperviscos > 0.0) {
output.write(" Hyper-viscosity coefficient: {:e}\n", hyperviscos);
dump.add(hyper_mu_x, "hyper_mu_x", 1);
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]

dump.add(hyper_mu_x, "hyper_mu_x", 1);
                                   ^
                                   true

return 1;
if (diffusion_par > 0.0) {
output.write(" diffusion_par: {:e}\n", diffusion_par);
dump.add(diffusion_par, "diffusion_par", 0);
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]

dump.add(diffusion_par, "diffusion_par", 0);
                                         ^
                                         false

// M: 4th order diffusion of p
if (diffusion_n4 > 0.0) {
output.write(" diffusion_n4: {:e}\n", diffusion_n4);
dump.add(diffusion_n4, "diffusion_n4", 0);
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]

dump.add(diffusion_n4, "diffusion_n4", 0);
                                       ^
                                       false

// M: 4th order diffusion of Ti
if (diffusion_ti4 > 0.0) {
output.write(" diffusion_ti4: {:e}\n", diffusion_ti4);
dump.add(diffusion_ti4, "diffusion_ti4", 0);
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]

dump.add(diffusion_ti4, "diffusion_ti4", 0);
                                         ^
                                         false

bendudson
bendudson previously approved these changes Dec 10, 2020
Copy link
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

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

Thanks @ZedThree !

MZ no longer has to be a power of two plus one

Co-authored-by: Ben Dudson <bd512@york.ac.uk>
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

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

// M: 4th order diffusion of Te
if (diffusion_te4 > 0.0) {
output.write(" diffusion_te4: {:e}\n", diffusion_te4);
dump.add(diffusion_te4, "diffusion_te4", 0);
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]

dump.add(diffusion_te4, "diffusion_te4", 0);
                                         ^
                                         false

// M: 4th order diffusion of Vipar
if (diffusion_v4 > 0.0) {
output.write(" diffusion_v4: {:e}\n", diffusion_v4);
dump.add(diffusion_v4, "diffusion_v4", 0);
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]

dump.add(diffusion_v4, "diffusion_v4", 0);
                                       ^
                                       false

// xqx: parallel hyper-viscous diffusion for vorticity
if (diffusion_u4 > 0.0) {
output.write(" diffusion_u4: {:e}\n", diffusion_u4);
dump.add(diffusion_u4, "diffusion_u4", 0);
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]

dump.add(diffusion_u4, "diffusion_u4", 0);
                                       ^
                                       false

BoutReal pnorm = max(P0, true); // Maximum over all processors
if (sink_vp > 0.0) {
output.write(" sink_vp(rate): {:e}\n", sink_vp);
dump.add(sink_vp, "sink_vp", 1);
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]

dump.add(sink_vp, "sink_vp", 1);
                             ^
                             true

vacuum_pressure *= pnorm; // Get pressure from fraction
vacuum_trans *= pnorm;
output.write(" sp_width(%%): {:e}\n", sp_width);
dump.add(sp_width, "sp_width", 1);
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]

dump.add(sp_width, "sp_width", 1);
                               ^
                               true

if (diffusion_u4 > 0.0) {
tmpA2.setBoundary("J");
}
dump.add(Xim_x, "Xim_x", 1);
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]

dump.add(Xim_x, "Xim_x", 1);
                         ^
                         true

tmpA2.setBoundary("J");
}
dump.add(Xim_x, "Xim_x", 1);
dump.add(Xim_z, "Xim_z", 1);
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]

dump.add(Xim_z, "Xim_z", 1);
                         ^
                         true

}
} else {
// Phi solved in RHS (explicitly)
dump.add(phi, "phi", 1);
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]

dump.add(phi, "phi", 1);
                     ^
                     true

* pow(Te_tmp * Tebar, -1.5); // nu_e in 1/S.
// nu_e.applyBoundary();
// mesh->communicate(nu_e);
BoutReal N_tmp1;
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 N_tmp1 is not initialized [cppcoreguidelines-init-variables]

BoutReal N_tmp1;
         ^
                = NAN

if (diffusion_par > 0.0) {
// xqx addition, begin
// Use Spitzer thermal conductivities
BoutReal Te_tmp1, Ti_tmp1;
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 Te_tmp1, Ti_tmp1;
^~~~~~~~~~~~~~~~~~~~~~~~~~

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

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

if (diffusion_par > 0.0) {
// xqx addition, begin
// Use Spitzer thermal conductivities
BoutReal Te_tmp1, Ti_tmp1;
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 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;
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 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;
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]

Field3D kappa_par_i_fl, kappa_par_e_fl;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#include <initialprofiles.hxx>

int reaction(BoutReal time);
class Split_operator : public PhysicsModel {
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: rate [cppcoreguidelines-pro-type-member-init]

class Split_operator : public PhysicsModel {
      ^


protected:
int init(bool restarting) override;
int rhs(BoutReal) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

int rhs(BoutReal) override;
                ^
                 /*UNUSED_t*/

// 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 {
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: 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 {
      ^

(globalOptions->getSection("ti"))->get("evolve", evolve_ti, true);
(globalOptions->getSection("Ajpar"))->get("evolve", evolve_ajpar, true);

if (ZeroElMass)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

if (ZeroElMass)
               ^
                {

ShearFactor = 0.0; // I disappears from metric
b0xcv.z += I*b0xcv.x;
}
bool ShiftXderivs;
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 ShiftXderivs is not initialized [cppcoreguidelines-init-variables]

bool ShiftXderivs;
     ^
                  = 0

mui_hat = nu_perp;
if (nu_perp < 1.e-10) {
mui_hat = (3. / 10.) * nuiix / wci;
} else
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

} else
      ^
       {

beta_p = 4.03e-11*Ni_x*Te_x/bmag/bmag;
if (estatic) {
beta_p = 1.e-29;
} else
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

} else
      ^
       {

@bendudson bendudson merged commit 7bdbeed into next Dec 14, 2020
@bendudson bendudson deleted the remove-legacy-models-with-fixes branch December 14, 2020 22:30
@ZedThree ZedThree mentioned this pull request Jan 4, 2021
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

2 participants