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

Do things automatically for FCI #3004

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

Do things automatically for FCI #3004

wants to merge 96 commits into from

Conversation

dschwoerer
Copy link
Contributor

Many things could be done automatically to make usage of FCI easier. However that may be slower, so this is optional.

dschwoerer and others added 30 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
Calculating parallel fields for metrics terms does not make sense.
Using such parallel fields is very, very likely a bug.
Just dumping the parallel slices does in general not work, as then
collect discards that, especially if NYPE==ny
hasBndryUpperY / hasBndryLowerY does not work for FCI and thus the
request does not make sense / can be configured to throw.
Thus it should not be checked if it is not needed.
@dschwoerer dschwoerer added the FCI label Oct 22, 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

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

#include "bout/sys/parallel_stencils.hxx"
#include "bout/sys/range.hxx"

class BoundaryRegionIter {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: destructor of 'BoundaryRegionIter' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]

class BoundaryRegionIter {
      ^
Additional context

include/bout/boundary_iterator.hxx:6: make it public and virtual

class BoundaryRegionIter {
      ^

include/bout/boundary_iterator.hxx Show resolved Hide resolved
include/bout/boundary_iterator.hxx Show resolved Hide resolved
: dir(bx + by), x(x), y(y), bx(bx), by(by), localmesh(mesh) {
ASSERT3(bx * by == 0);
}
bool operator!=(const BoundaryRegionIter& rhs) { return ind() != rhs.ind(); }
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 'operator!=' can be made const [readability-make-member-function-const]

Suggested change
bool operator!=(const BoundaryRegionIter& rhs) { return ind() != rhs.ind(); }
bool operator!=(const BoundaryRegionIter& rhs) const { return ind() != rhs.ind(); }

BoutReal& yprev(Field3D& f) const { return f[ind().yp(-by).xp(-bx)]; }
const BoutReal& yprev(const Field3D& f) const { return f[ind().yp(-by).xp(-bx)]; }

const int dir;
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 'dir' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int dir;
            ^

#if BOUT_USE_FCI_AUTOMAGIC
if (var.isFci()) {
for (size_t i = 0; i < result.numberParallelSlices(); ++i) {
BOUT_FOR(d, result.yup(i).getRegion(rgn)) {
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 'd' is too short, expected at least 2 characters [readability-identifier-length]

{
                 ^

result.yup(i)[d] = f;
}
}
BOUT_FOR(d, result.ydown(i).getRegion(rgn)) {
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 'd' is too short, expected at least 2 characters [readability-identifier-length]

}
                 ^

bool hasParallelSlices() const { return true; }
void calcParallelSlices() const {}
void clearParallelSlices() {}
int numberParallelSlices() { return 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: method 'numberParallelSlices' can be made static [readability-convert-member-functions-to-static]

Suggested change
int numberParallelSlices() { return 0; }
static int numberParallelSlices() { return 0; }

@@ -26,6 +26,15 @@

#include "bout/mask.hxx"

#define USE_NEW_WEIGHTS 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: macro 'USE_NEW_WEIGHTS' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]

#define USE_NEW_WEIGHTS 1
        ^

@@ -26,6 +26,15 @@

#include "bout/mask.hxx"

#define USE_NEW_WEIGHTS 1
#if BOUT_HAS_PETSC
#define HS_USE_PETSC 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: macro 'HS_USE_PETSC' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]

#define HS_USE_PETSC 1
        ^

Otherwise it is attempted to transform it to field-aligned, which
fails as the parallel transform is not set.
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 93. Check the log or trigger a new build to see more.

#if BOUT_USE_FCI_AUTOMAGIC
if (var.isFci()) {
for (size_t i = 0; i < result.numberParallelSlices(); ++i) {
BOUT_FOR(d, result.yup(i).getRegion(rgn)) {
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 'd' is too short, expected at least 2 characters [readability-identifier-length]

{
                   ^

result.yup(i)[d] = f;
}
}
BOUT_FOR(d, result.ydown(i).getRegion(rgn)) {
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 'd' is too short, expected at least 2 characters [readability-identifier-length]

}
                   ^

@@ -43,57 +52,52 @@ public:
protected:
Mesh* localmesh{nullptr};

std::string region_name;
std::shared_ptr<Region<Ind3D>> region{nullptr};
int region_id{-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: member variable 'region_id' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  int region_id{-1};
      ^

Mesh* mesh = nullptr)
: y_offset(y_offset), localmesh(mesh), region(std::move(region)) {}
: y_offset(y_offset), localmesh(mesh),
region_id(localmesh->getRegionID(region_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]

        region_id(localmesh->getRegionID(region_name)) {}
                  ^

void setRegion(const std::shared_ptr<Region<Ind3D>>& region) {
this->region_name = "";
this->region = region;
this->region_id = localmesh->getRegionID(region_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(region_name);
                      ^

}

// extrapolate the gradient into the boundary
inline BoutReal extrapolate_grad_o1(const Field3D& f) const { return 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: unused parameter 'f' [clang-diagnostic-unused-parameter]

  inline BoutReal extrapolate_grad_o1(const Field3D& f) const { return 0; }
                                                     ^

}

// extrapolate the gradient into the boundary
inline BoutReal extrapolate_grad_o1(const Field3D& f) const { return 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 'f' is unused [misc-unused-parameters]

Suggested change
inline BoutReal extrapolate_grad_o1(const Field3D& f) const { return 0; }
inline BoutReal extrapolate_grad_o1(const Field3D& /*f*/) const { return 0; }


private:
const IndicesVec& bndry_points;
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 'bndry_points' of type 'const IndicesVec &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

  const IndicesVec& bndry_points;
                    ^


private:
const IndicesVec& bndry_points;
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 'bndry_points' of type 'const std::vectorbout::parallel_boundary_region::Indices &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

  const IndicesVec& bndry_points;
                    ^

const BoutReal& yprev(const Field3D& f) const { return f.ynext(-dir)[ind().yp(-dir)]; }
BoutReal& yprev(Field3D& f) const { return f.ynext(-dir)[ind().yp(-dir)]; }
public:
const int dir;
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 'dir' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int dir;
            ^

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

const BoutReal& yprev(const Field3D& f) const { return f.ynext(-dir)[ind().yp(-dir)]; }
BoutReal& yprev(Field3D& f) const { return f.ynext(-dir)[ind().yp(-dir)]; }
public:
const int dir;
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 'dir' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  const int dir;
            ^

BoutReal& yprev(Field3D& f) const { return f.ynext(-dir)[ind().yp(-dir)]; }
public:
const int dir;
Mesh* 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: member variable 'localmesh' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  Mesh* localmesh;
        ^

return std::binary_search(std::begin(bndry_points), std::end(bndry_points),
*bndry.bndry_position,
[](const bout::parallel_boundary_region::Indices& i1,
const bout::parallel_boundary_region::Indices& i2) {
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 'i2' is too short, expected at least 3 characters [readability-identifier-length]

                                 const bout::parallel_boundary_region::Indices& i2) {
                                                                                ^

return BoundaryRegionParIter(bndry_points, bndry_points.end(), dir, localmesh);
}

const int dir;
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 'dir' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int dir;
            ^

return BoundaryRegionParIter(bndry_points, bndry_points.end(), dir, localmesh);
}

const int dir;
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 'dir' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  const int dir;
            ^

BoutReal value1) {
return (spacing0 * value1 - spacing1 * value0) / (spacing0 - spacing1);
}
inline BoutReal neumann_o2(BoutReal UNUSED(spacing0), BoutReal value0, BoutReal spacing1,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unknown type name 'BoutReal' [clang-diagnostic-error]

inline BoutReal neumann_o2(BoutReal UNUSED(spacing0), BoutReal value0, BoutReal spacing1,
                                                      ^

BoutReal value1) {
return (spacing0 * value1 - spacing1 * value0) / (spacing0 - spacing1);
}
inline BoutReal neumann_o2(BoutReal UNUSED(spacing0), BoutReal value0, BoutReal spacing1,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unknown type name 'BoutReal' [clang-diagnostic-error]

inline BoutReal neumann_o2(BoutReal UNUSED(spacing0), BoutReal value0, BoutReal spacing1,
                                                                       ^

return (spacing0 * value1 - spacing1 * value0) / (spacing0 - spacing1);
}
inline BoutReal neumann_o2(BoutReal UNUSED(spacing0), BoutReal value0, BoutReal spacing1,
BoutReal value1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unknown type name 'BoutReal' [clang-diagnostic-error]

                           BoutReal value1) {
                           ^

BoutReal value1) {
return -spacing1 * value0 + value1;
}
inline BoutReal dirichlet_o3(BoutReal spacing0, BoutReal value0, BoutReal spacing1,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unknown type name 'BoutReal' [clang-diagnostic-error]

inline BoutReal dirichlet_o3(BoutReal spacing0, BoutReal value0, BoutReal spacing1,
       ^

@@ -39,3 +39,14 @@ int Field::getNx() const { return getMesh()->LocalNx; }
int Field::getNy() const { return getMesh()->LocalNy; }

int Field::getNz() const { return getMesh()->LocalNz; }

bool Field::isFci() const {
const auto coords = this->getCoordinates();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'const auto coords' can be declared as 'auto *const coords' [readability-qualified-auto]

Suggested change
const auto coords = this->getCoordinates();
auto *const coords = this->getCoordinates();

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

{
std::vector<Field3D> fields{dx, dy, dz, g11, g22, g33, g12, g13,
g23, g_11, g_22, g_33, g_12, g_13, g_23, J};
for (auto& f : fields) {
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 'f' is too short, expected at least 3 characters [readability-identifier-length]

    for (auto& f : fields) {
               ^

private:
// number of procs
BoutMesh* mesh;
const int nxpe;
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 'nxpe' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int nxpe;
            ^

// number of procs
BoutMesh* mesh;
const int nxpe;
const int nype;
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 'nype' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int nype;
            ^

BoutMesh* mesh;
const int nxpe;
const int nype;
const int nzpe{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: member 'nzpe' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int nzpe{1};
            ^

const int nxpe;
const int nype;
const int nzpe{1};
const int xstart, ystart, zstart;
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 'xstart' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

  const int xstart, ystart, zstart;
            ^

// x, z, i_corn, k_corner(x, y, z));
// ixstep = mesh->LocalNx * mesh->LocalNz;
for (int j = 0; j < 4; ++j) {
PetscInt idxm[4];
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 idxm[4];
      ^

// ixstep = mesh->LocalNx * mesh->LocalNz;
for (int j = 0; j < 4; ++j) {
PetscInt idxm[4];
PetscScalar vals[4];
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]

      PetscScalar vals[4];
      ^

PetscInt idxm[4];
PetscScalar vals[4];
for (int k = 0; k < 4; ++k) {
idxm[k] = conv.fromMeshToGlobal(i_corn - 1 + j, y + y_offset,
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 use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]

        idxm[k] = conv.fromMeshToGlobal(i_corn - 1 + j, y + y_offset,
        ^

for (int k = 0; k < 4; ++k) {
idxm[k] = conv.fromMeshToGlobal(i_corn - 1 + j, y + y_offset,
k_corner(x, y, z) - 1 + k);
vals[k] = newWeights[j * 4 + k][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: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]

        vals[k] = newWeights[j * 4 + k][i];
        ^

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);
                                    ^

@dschwoerer
Copy link
Contributor Author

test-laplace-petsc3d is failing, I am not sure why.
I tried changing the PETSc options, but that does not seem to impact the error.

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.

1 participant