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

Allow X splitting with FCI #2651

Open
wants to merge 48 commits into
base: next
Choose a base branch
from
Open

Allow X splitting with FCI #2651

wants to merge 48 commits into from

Conversation

dschwoerer
Copy link
Contributor

Uses PETSc. Currently only implemented for hermitespline.

dschwoerer and others added 21 commits February 8, 2023 10:05
Might help with performance
Otherwise the regions aren't cached.
Might help with performance
Precalculate the matrix. This hard-codes the DDX and DDZ derivative to
C2.

Rather than taking derivatives, this first does the matrix-matrix
multiplication and then does only a single matrix-vector operation,
rather than 4.

Retains a switch to go back to the old version.
release leaks the pointer, reset free's the object.
* helped finding the bug for the boundary
* rather slow (around 1 minute)
* needs internet connectivity
@dschwoerer
Copy link
Contributor Author

Requires #2324

@dschwoerer dschwoerer changed the title Allow X splitting with FCI [blocked] Allow X splitting with FCI Feb 8, 2023
@dschwoerer dschwoerer closed this Feb 8, 2023
@dschwoerer dschwoerer changed the title [blocked] Allow X splitting with FCI Allow X splitting with FCI Feb 8, 2023
@dschwoerer dschwoerer reopened this Feb 8, 2023
@dschwoerer dschwoerer marked this pull request as draft February 8, 2023 13:10
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 51. Check the log or trigger a new build to see more.

this->region_name = "";
this->region = std::make_shared<Region<Ind3D>>(region);
std::string name;
int i = 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: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]

    int i = 0;
        ^

this->region = std::make_shared<Region<Ind3D>>(region);
std::string name;
int i = 0;
do {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]

    do {
    ^

name = fmt::format("unsec_reg_xz_interp_{:d}", i++);
} while (localmesh->hasRegion3D(name));
localmesh->addRegion(name, region);
this->region_id = localmesh->getRegionID(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]

    this->region_id = localmesh->getRegionID(name);
                      ^

return localmesh->getRegion(region);
}
ASSERT1(has_region);
return getRegion();
if (region == "" or region == "RGN_ALL") {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object [readability-container-size-empty]

Suggested change
if (region == "" or region == "RGN_ALL") {
if (region.empty() or region == "RGN_ALL") {
Additional context

/usr/include/c++/12/bits/basic_string.h:1190: method 'basic_string'::empty() defined here

      empty() const _GLIBCXX_NOEXCEPT
      ^

@@ -134,8 +138,8 @@ protected:
/// This is protected rather than private so that it can be
/// extended and used by HermiteSplineMonotonic

Tensor<int> i_corner; // x-index of bottom-left grid point
Tensor<int> k_corner; // z-index of bottom-left grid point
Tensor<SpecificInd<IND_TYPE::IND_3D>> i_corner; // index of bottom-left grid point
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 'i_corner' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  Tensor<SpecificInd<IND_TYPE::IND_3D>> i_corner; // index of bottom-left grid point
                                        ^

newWeights[w].allocate();
}
#ifdef HS_USE_PETSC
petsclib = new PetscLib(
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: assigning newly created 'gsl::owner<>' to non-owner 'PetscLib *' [cppcoreguidelines-owning-memory]

  petsclib = new PetscLib(
  ^

// PetscInt N, PetscInt d_nz, const PetscInt d_nnz[],
// PetscInt o_nz, const PetscInt o_nnz[], Mat *A)
// MatSetSizes(Mat A,PetscInt m,PetscInt n,PetscInt M,PetscInt N)
const int m = mesh->LocalNx * mesh->LocalNy * mesh->LocalNz;
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 name 'm' is too short, expected at least 3 characters [readability-identifier-length]

  const int m = mesh->LocalNx * mesh->LocalNy * mesh->LocalNz;
            ^

// PetscInt o_nz, const PetscInt o_nnz[], Mat *A)
// MatSetSizes(Mat A,PetscInt m,PetscInt n,PetscInt M,PetscInt N)
const int m = mesh->LocalNx * mesh->LocalNy * mesh->LocalNz;
const int M = m * mesh->getNXPE() * mesh->getNYPE();
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 name 'M' is too short, expected at least 3 characters [readability-identifier-length]

  const int M = m * mesh->getNXPE() * mesh->getNYPE();
            ^


h11_x(x, y, z) = (t_x * t_x * t_x) - (t_x * t_x);
h11_z(x, y, z) = (t_z * t_z * t_z) - (t_z * t_z);
for (int w = 0; w < 16; ++w) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: loop variable name 'w' is too short, expected at least 2 characters [readability-identifier-length]

    for (int w = 0; w < 16; ++w) {
             ^

newWeights[7][i] -= h11_x[i] * h11_z[i] / 4;
newWeights[5][i] += h11_x[i] * h11_z[i] / 4;
#ifdef HS_USE_PETSC
PetscInt idxn[1] = {conv.fromLocalToGlobal(x, y + y_offset, z)};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

    PetscInt idxn[1] = {conv.fromLocalToGlobal(x, y + y_offset, z)};
    ^

@dschwoerer
Copy link
Contributor Author

This PR requires par-bc-cleanup #2629

It uses also the updated boundary condition name (parallel_neumann_o2 instead of parallel_neumann) which I think is introduced in #2649 but that would be easy to revert.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file needed? It looks like NumPy documentation. Also the html version.

@bendudson
Copy link
Contributor

Thanks @dschwoerer ! The testing is nice. A couple of questions:

  1. Would you mind if I split the hermitespline XZ interpolation into two implementations? It would help keep the PETSc implementation cleaner if it didn't have #ifdefs throughout. I started this in WIP: XZ interpolation using PETSc #2858. Would you mind if I made a PR into this branch?
  2. Do I interpret this right that the PETSc global index includes the communication guard cells (i.e. two global indices correspond to conceptually the same cell, but on different processors)? I think this is ok, and am just trying to work out what sensible ways there are to define a global cell index.

@dschwoerer
Copy link
Contributor Author

1. Would you mind if I split the `hermitespline` XZ interpolation into two implementations? It would help keep the PETSc implementation cleaner if it didn't have #ifdefs throughout. I started this in [WIP: XZ interpolation using PETSc #2858](https://github.com/boutproject/BOUT-dev/pull/2858). Would you mind if I made a PR into this branch?

Not at all!
I thought it would not be needed, but if you can use it, sure 👍

2. Do I interpret this right that the PETSc global index includes the communication guard cells (i.e. two global indices correspond to conceptually the same cell, but on different processors)? I think this is ok, and am just trying to work out what sensible ways there are to define a global cell index.

Yes, that is the case. The motivation is that a PETScVector can use directly a Field3D, and I do not have to shuffle the data around. I am not sure about the performance implication, but I think it also avoids some copying. At least it made it easier for me to think about the PETSc Vector, if it has the same memory layout then the Field3D.

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 41. Check the log or trigger a new build to see more.


virtual BoundaryOpPar* clone(BoundaryRegionPar* region,
const std::list<std::string>& args) = 0;
virtual BoundaryOpPar* clone(BoundaryRegionPar* region, Field3D* f) = 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: parameter name 'f' is too short, expected at least 3 characters [readability-identifier-length]

  virtual BoundaryOpPar* clone(BoundaryRegionPar* region, Field3D* f) = 0;
                                                                   ^

BoundaryRegionPar* bndry{nullptr};

protected:
/// Possible ways to get boundary values
std::shared_ptr<FieldGenerator> gen_values;
Field3D* field_values;
Field3D* field_values{nullptr};
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 'field_values' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  Field3D* field_values{nullptr};
           ^

}
}

return new T(region);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: returning a newly created resource of type 'BoundaryOpPar *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' [cppcoreguidelines-owning-memory]

    return new T(region);
    ^

return new T(region);
}

BoundaryOpPar* clone(BoundaryRegionPar* region, Field3D* f) 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: parameter name 'f' is too short, expected at least 3 characters [readability-identifier-length]

  BoundaryOpPar* clone(BoundaryRegionPar* region, Field3D* f) override {
                                                           ^

}

BoundaryOpPar* clone(BoundaryRegionPar* region, Field3D* f) override {
return new T(region, 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: returning a newly created resource of type 'BoundaryOpPar *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' [cppcoreguidelines-owning-memory]

    return new T(region, f);
    ^

@@ -38,7 +14,7 @@ BoutReal BoundaryOpPar::getValue(const BoundaryRegionPar& bndry, BoutReal t) {
switch (value_type) {
case ValueType::GEN:
return gen_values->generate(
Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
bout::generator::Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'BoutReal' (aka 'double') to 'int' [bugprone-narrowing-conversions]

        bout::generator::Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
                                 ^

@@ -38,7 +14,7 @@ BoutReal BoundaryOpPar::getValue(const BoundaryRegionPar& bndry, BoutReal t) {
switch (value_type) {
case ValueType::GEN:
return gen_values->generate(
Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
bout::generator::Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'BoutReal' (aka 'double') to 'int' [bugprone-narrowing-conversions]

        bout::generator::Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
                                            ^

@@ -38,7 +14,7 @@ BoutReal BoundaryOpPar::getValue(const BoundaryRegionPar& bndry, BoutReal t) {
switch (value_type) {
case ValueType::GEN:
return gen_values->generate(
Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
bout::generator::Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'BoutReal' (aka 'double') to 'int' [bugprone-narrowing-conversions]

        bout::generator::Context(bndry.s_x, bndry.s_y, bndry.s_z, CELL_CENTRE, mesh, t));
                                                       ^

@@ -244,7 +139,7 @@ void BoundaryOpPar_neumann::apply(Field3D& f, BoutReal t) {
int z = bndry->z;

// Generate the boundary value
BoutReal value = getValue(x, y, z, t);
BoutReal value = getValue(*bndry, t);
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 'value' of type 'BoutReal' (aka 'double') can be declared 'const' [misc-const-correctness]

Suggested change
BoutReal value = getValue(*bndry, t);
BoutReal const value = getValue(*bndry, t);

@@ -45,5 +45,8 @@ Context::Context(const BoundaryRegion* bndry, int iz, CELL_LOC loc, BoutReal t,
parameters["t"] = t;
}

Context::Context(BoutReal x, BoutReal y, BoutReal z, Mesh* msh, BoutReal t)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter name 't' is too short, expected at least 3 characters [readability-identifier-length]

Context::Context(BoutReal x, BoutReal y, BoutReal z, Mesh* msh, BoutReal t)
                                                                         ^

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

{
using bout::globals::mesh;
Options* options = Options::getRoot();
int i = 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: variable name 'i' is too short, expected at least 3 characters [readability-identifier-length]

    int i = 0;
        ^

using bout::globals::mesh;
Options* options = Options::getRoot();
int i = 0;
std::string default_str{"not_set"};
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 'default_str' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change
std::string default_str{"not_set"};
std::string const default_str{"not_set"};

mesh->communicate(input);
input.applyParallelBoundary("parallel_neumann_o2");
for (int slice = -mesh->ystart; slice <= mesh->ystart; ++slice) {
if (slice) {
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]

Suggested change
if (slice) {
if (slice != 0) {

dx = index.x() - dice();
{
// Random number generator
std::default_random_engine generator;
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 'generator' of type 'std::default_random_engine' (aka 'linear_congruential_engine<unsigned long, 16807UL, 0UL, 2147483647UL>') can be declared 'const' [misc-const-correctness]

Suggested change
std::default_random_engine generator;
std::default_random_engine const generator;

// Random number generator
std::default_random_engine generator;
// Uniform distribution of BoutReals from 0 to 1
std::uniform_real_distribution<BoutReal> distribution{0.0, 1.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: variable 'distribution' of type 'std::uniform_real_distribution' (aka 'uniform_real_distribution') can be declared 'const' [misc-const-correctness]

Suggested change
std::uniform_real_distribution<BoutReal> distribution{0.0, 1.0};
std::uniform_real_distribution<BoutReal> const distribution{0.0, 1.0};


std::string c_func;
auto c_gen = getGeneratorFromOptions("c", c_func);
Field3D c = f.create3D(c_func);
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 'c' of type 'Field3D' can be declared 'const' [misc-const-correctness]

Suggested change
Field3D c = f.create3D(c_func);
Field3D const c = f.create3D(c_func);


std::string c_func;
auto c_gen = getGeneratorFromOptions("c", c_func);
Field3D c = f.create3D(c_func);
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 name 'c' is too short, expected at least 3 characters [readability-identifier-length]

    Field3D c = f.create3D(c_func);
            ^


for (const auto& index : deltax) {
// Get some random displacements
BoutReal dx = index.x() + dice();
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 name 'dx' is too short, expected at least 3 characters [readability-identifier-length]

      BoutReal dx = index.x() + dice();
               ^

for (const auto& index : deltax) {
// Get some random displacements
BoutReal dx = index.x() + dice();
BoutReal dz = index.z() + dice();
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 'dz' of type 'BoutReal' (aka 'double') can be declared 'const' [misc-const-correctness]

Suggested change
BoutReal dz = index.z() + dice();
BoutReal const dz = index.z() + dice();

for (const auto& index : deltax) {
// Get some random displacements
BoutReal dx = index.x() + dice();
BoutReal dz = index.z() + dice();
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 name 'dz' is too short, expected at least 3 characters [readability-identifier-length]

      BoutReal dz = index.z() + dice();
               ^

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

region = regionFromMask(mask, localmesh);
setRegion(regionFromMask(mask, localmesh));
}
~XZHermiteSpline() {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
~XZHermiteSpline() {
~XZHermiteSpline() override {

const int nype;
const int nzpe{1};
const int xstart, ystart, zstart;
const int lnx, lny, lnz;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: private field 'lnx' is not used [clang-diagnostic-unused-private-field]

  const int lnx, lny, lnz;
            ^

const int nype;
const int nzpe{1};
const int xstart, ystart, zstart;
const int lnx, lny, lnz;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: private field 'lny' is not used [clang-diagnostic-unused-private-field]

  const int lnx, lny, lnz;
                 ^

const int nype;
const int nzpe{1};
const int xstart, ystart, zstart;
const int lnx, lny, lnz;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: private field 'lnz' is not used [clang-diagnostic-unused-private-field]

  const int lnx, lny, lnz;
                      ^

k_corner(x, y, z) - 1 + k);
vals[k] = newWeights[j * 4 + k][i];
}
MatSetValues(petscWeights, 1, idxn, 4, idxm, vals, INSERT_VALUES);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

      MatSetValues(petscWeights, 1, idxn, 4, idxm, vals, INSERT_VALUES);
                                    ^

k_corner(x, y, z) - 1 + k);
vals[k] = newWeights[j * 4 + k][i];
}
MatSetValues(petscWeights, 1, idxn, 4, idxm, vals, INSERT_VALUES);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

      MatSetValues(petscWeights, 1, idxn, 4, idxm, vals, INSERT_VALUES);
                                             ^

k_corner(x, y, z) - 1 + k);
vals[k] = newWeights[j * 4 + k][i];
}
MatSetValues(petscWeights, 1, idxn, 4, idxm, vals, INSERT_VALUES);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

      MatSetValues(petscWeights, 1, idxn, 4, idxm, vals, INSERT_VALUES);
                                                   ^

mesh may not be initialised or be a different mesh
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

// PetscInt N, PetscInt d_nz, const PetscInt d_nnz[],
// PetscInt o_nz, const PetscInt o_nnz[], Mat *A)
// MatSetSizes(Mat A,PetscInt m,PetscInt n,PetscInt M,PetscInt N)
const int m = localmesh->LocalNx * localmesh->LocalNy * localmesh->LocalNz;
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 name 'm' is too short, expected at least 3 characters [readability-identifier-length]

  const int m = localmesh->LocalNx * localmesh->LocalNy * localmesh->LocalNz;
            ^

// PetscInt o_nz, const PetscInt o_nnz[], Mat *A)
// MatSetSizes(Mat A,PetscInt m,PetscInt n,PetscInt M,PetscInt N)
const int m = localmesh->LocalNx * localmesh->LocalNy * localmesh->LocalNz;
const int M = m * localmesh->getNXPE() * localmesh->getNYPE();
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 name 'M' is too short, expected at least 3 characters [readability-identifier-length]

  const int M = m * localmesh->getNXPE() * localmesh->getNYPE();
            ^

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

// Set up generators and solutions for three different analtyic functions
std::string a_func;
auto a_gen = getGeneratorFromOptions("a", a_func);
const Field3D a = fieldfact.create3D(a_func);
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 name 'a' is too short, expected at least 3 characters [readability-identifier-length]

    const Field3D a = fieldfact.create3D(a_func);
                  ^


std::string b_func;
auto b_gen = getGeneratorFromOptions("b", b_func);
const Field3D b = fieldfact.create3D(b_func);
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 name 'b' is too short, expected at least 3 characters [readability-identifier-length]

    const Field3D b = fieldfact.create3D(b_func);
                  ^


std::string c_func;
auto c_gen = getGeneratorFromOptions("c", c_func);
const Field3D c = fieldfact.create3D(c_func);
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 name 'c' is too short, expected at least 3 characters [readability-identifier-length]

    const Field3D c = fieldfact.create3D(c_func);
                  ^

@dschwoerer dschwoerer added the FCI label Mar 19, 2024
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

public:
XZHermiteSpline(Mesh* mesh = nullptr) : XZHermiteSpline(0, mesh) {}
XZHermiteSpline(int y_offset = 0, Mesh* mesh = nullptr);
XZHermiteSpline(const BoutMask& mask, int y_offset = 0, Mesh* mesh = nullptr)
: XZHermiteSpline(y_offset, mesh) {
region = regionFromMask(mask, localmesh);
setRegion(regionFromMask(mask, localmesh));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching member function for call to 'setRegion' [clang-diagnostic-error]

    setRegion(regionFromMask(mask, localmesh));
    ^
Additional context

include/bout/interpolation_xz.hxx:77: candidate function not viable: no known conversion from 'XZHermiteSpline' to 'XZInterpolation' for object argument

  void setRegion(const std::unique_ptr<Region<Ind3D>> region) { setRegion(*region); }
       ^

include/bout/interpolation_xz.hxx:74: candidate function not viable: no known conversion from 'XZHermiteSpline' to 'XZInterpolation' for object argument

  void setRegion(const std::string& region_name) {
       ^

include/bout/interpolation_xz.hxx:78: candidate function not viable: no known conversion from 'XZHermiteSpline' to 'XZInterpolation' for object argument

  void setRegion(const Region<Ind3D>& region) {
       ^

@@ -213,7 +249,7 @@
XZLagrange4pt(int y_offset = 0, Mesh* mesh = nullptr);
XZLagrange4pt(const BoutMask& mask, int y_offset = 0, Mesh* mesh = nullptr)
: XZLagrange4pt(y_offset, mesh) {
region = regionFromMask(mask, localmesh);
setRegion(regionFromMask(mask, localmesh));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching member function for call to 'setRegion' [clang-diagnostic-error]

    setRegion(regionFromMask(mask, localmesh));
    ^
Additional context

include/bout/interpolation_xz.hxx:77: candidate function not viable: no known conversion from 'XZLagrange4pt' to 'XZInterpolation' for object argument

  void setRegion(const std::unique_ptr<Region<Ind3D>> region) { setRegion(*region); }
       ^

include/bout/interpolation_xz.hxx:74: candidate function not viable: no known conversion from 'XZLagrange4pt' to 'XZInterpolation' for object argument

  void setRegion(const std::string& region_name) {
       ^

include/bout/interpolation_xz.hxx:78: candidate function not viable: no known conversion from 'XZLagrange4pt' to 'XZInterpolation' for object argument

  void setRegion(const Region<Ind3D>& region) {
       ^

@@ -246,7 +282,7 @@
XZBilinear(int y_offset = 0, Mesh* mesh = nullptr);
XZBilinear(const BoutMask& mask, int y_offset = 0, Mesh* mesh = nullptr)
: XZBilinear(y_offset, mesh) {
region = regionFromMask(mask, localmesh);
setRegion(regionFromMask(mask, localmesh));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching member function for call to 'setRegion' [clang-diagnostic-error]

    setRegion(regionFromMask(mask, localmesh));
    ^
Additional context

include/bout/interpolation_xz.hxx:77: candidate function not viable: no known conversion from 'XZBilinear' to 'XZInterpolation' for object argument

  void setRegion(const std::unique_ptr<Region<Ind3D>> region) { setRegion(*region); }
       ^

include/bout/interpolation_xz.hxx:74: candidate function not viable: no known conversion from 'XZBilinear' to 'XZInterpolation' for object argument

  void setRegion(const std::string& region_name) {
       ^

include/bout/interpolation_xz.hxx:78: candidate function not viable: no known conversion from 'XZBilinear' to 'XZInterpolation' for object argument

  void setRegion(const Region<Ind3D>& region) {
       ^

// ASSERT2(i.ny == n2);
ASSERT2(0 <= i.ind && i.ind < n1 * n2 * n3);
return data[i.ind];
}

T& operator[](Ind3D 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: class member cannot be redeclared [clang-diagnostic-error]

  T& operator[](Ind3D i) {
     ^
Additional context

include/bout/utils.hxx:363: previous definition is here

  T& operator[](Ind3D i) {
     ^

#ifdef HS_USE_PETSC
IndConverter conv{localmesh};
#endif
BOUT_FOR(i, getRegion(region)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching member function for call to 'getRegion' [clang-diagnostic-error]

  BOUT_FOR(i, getRegion(region)) {
              ^
Additional context

include/bout/region.hxx:135: expanded from macro 'BOUT_FOR'

  BOUT_FOR_OMP(index, region, parallel for schedule(BOUT_OPENMP_SCHEDULE))
                      ^

include/bout/region.hxx:131: expanded from macro 'BOUT_FOR_OMP'

#define BOUT_FOR_OMP(index, region, omp_pragmas) BOUT_FOR_SERIAL(index, region)
                                                                        ^

include/bout/region.hxx:119: expanded from macro 'BOUT_FOR_SERIAL'

  for (auto block = region.getBlocks().cbegin(), end = region.getBlocks().cend(); \
                    ^

include/bout/interpolation_xz.hxx:91: candidate function not viable: no known conversion from 'XZHermiteSpline' to 'const XZInterpolation' for object argument

  const Region<Ind3D>& getRegion(const std::string& region) const {
                       ^

include/bout/interpolation_xz.hxx:87: candidate function not viable: requires 0 arguments, but 1 was provided

  const Region<Ind3D>& getRegion() const {
                       ^

@@ -59,67 +59,50 @@ Field3D XZMonotonicHermiteSpline::interpolate(const Field3D& f,

const auto curregion{getRegion(region)};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching member function for call to 'getRegion' [clang-diagnostic-error]

  const auto curregion{getRegion(region)};
                       ^
Additional context

include/bout/interpolation_xz.hxx:91: candidate function not viable: no known conversion from 'const XZMonotonicHermiteSpline' to 'const XZInterpolation' for object argument

  const Region<Ind3D>& getRegion(const std::string& region) const {
                       ^

include/bout/interpolation_xz.hxx:87: candidate function not viable: requires 0 arguments, but 1 was provided

  const Region<Ind3D>& getRegion() const {
                       ^

ZedThree added a commit that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants