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

WIP: XZ interpolation using PETSc #2858

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

Conversation

bendudson
Copy link
Contributor

Work in progress

Intend to adapt D.Bold's PETSc interpolation for yup/ydown, for merging into next.

Reduce duplication by moving some overloads to the base class.
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

include/bout/interpolation_xz.hxx Show resolved Hide resolved
include/bout/interpolation_xz.hxx Show resolved Hide resolved
include/bout/interpolation_xz.hxx Show resolved Hide resolved
include/bout/interpolation_xz.hxx Show resolved Hide resolved
include/bout/interpolation_xz.hxx Show resolved Hide resolved
include/bout/interpolation_xz.hxx Show resolved Hide resolved
@@ -208,6 +209,10 @@

Field3D t_x, t_z;

BoutReal lagrange_4pt(BoutReal v2m, BoutReal vm, BoutReal vp, BoutReal v2p,
BoutReal offset) const;
BoutReal lagrange_4pt(const BoutReal v[], BoutReal offset) const;
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]

  BoutReal lagrange_4pt(const BoutReal v[], BoutReal offset) const;
                              ^

include/bout/interpolation_xz.hxx Show resolved Hide resolved
Optimised build was omitting XZInterpolation factory registrations.
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

XZInterpolationFactory::RegisterUnavailableInFactory;

namespace {
RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
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 'registerinterphermitespline' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
                                         ^

XZInterpolationFactory::RegisterUnavailableInFactory;

namespace {
RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
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 'registerinterphermitespline' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]

RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
                                         ^


namespace {
RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
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 'registerinterpmonotonichermitespline' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
                                                  ^


namespace {
RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
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 'registerinterpmonotonichermitespline' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]

RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
                                                  ^

RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
"monotonichermitespline"};
RegisterXZInterpolation<XZLagrange4pt> registerinterplagrange4pt{"lagrange4pt"};
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 'registerinterplagrange4pt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

RegisterXZInterpolation<XZLagrange4pt> registerinterplagrange4pt{"lagrange4pt"};
                                       ^

RegisterXZInterpolation<XZHermiteSpline> registerinterphermitespline{"hermitespline"};
RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
"monotonichermitespline"};
RegisterXZInterpolation<XZLagrange4pt> registerinterplagrange4pt{"lagrange4pt"};
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 'registerinterplagrange4pt' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]

RegisterXZInterpolation<XZLagrange4pt> registerinterplagrange4pt{"lagrange4pt"};
                                       ^

RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
"monotonichermitespline"};
RegisterXZInterpolation<XZLagrange4pt> registerinterplagrange4pt{"lagrange4pt"};
RegisterXZInterpolation<XZBilinear> registerinterpbilinear{"bilinear"};
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 'registerinterpbilinear' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

RegisterXZInterpolation<XZBilinear> registerinterpbilinear{"bilinear"};
                                    ^

RegisterXZInterpolation<XZMonotonicHermiteSpline> registerinterpmonotonichermitespline{
"monotonichermitespline"};
RegisterXZInterpolation<XZLagrange4pt> registerinterplagrange4pt{"lagrange4pt"};
RegisterXZInterpolation<XZBilinear> registerinterpbilinear{"bilinear"};
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 'registerinterpbilinear' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]

RegisterXZInterpolation<XZBilinear> registerinterpbilinear{"bilinear"};
                                    ^

@dschwoerer
Copy link
Contributor

Hi Ben,
just to make sure you did not miss the PR, here is my PETSc implementation as PR for next:
#2651

The FCI MMS tests are passing, thus I am not sure the issue I am seeing in hermes-2 with X-splitting is actually related to the PETSc part, or some other part of the code that has hard-coded the assumption that there is no splitting in X.

@bendudson
Copy link
Contributor Author

Hi @dschwoerer ! Thanks yes; I'm going through your PR in steps so I can (a) understand it myself; (b) implement the PETSc method as a separate implementation of XZInterpolate. As I'm going through it I'm trying to work out where the no-X-splitting assumptions are being made.

Comment on lines +41 to +43
// Cast to base pointer so virtual function overload is resolved
return static_cast<XZInterpolation*>(&interpolateMethod)
->interpolate(f, delta_x, delta_z);
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed -- the issue is probably because we now need a using XZInterpolation::interpolate in the derived classes to bring in the missing overloads of the virtual function.

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.

3 participants