Skip to content

New Y-Boundaries for unified sheath code#3308

Draft
dschwoerer wants to merge 134 commits into
nextfrom
new-par-bc-unified
Draft

New Y-Boundaries for unified sheath code#3308
dschwoerer wants to merge 134 commits into
nextfrom
new-par-bc-unified

Conversation

@dschwoerer
Copy link
Copy Markdown
Contributor

This allows to implement unified BCs for yup/ydown as well as FCI / FA.
Taking into acount things like whether the boundary is next to the domain, or between the parallel slices.

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

Comment thread include/bout/boundary_iterator.hxx
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Copy link
Copy Markdown
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 85. Check the log or trigger a new build to see more.

Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Copy link
Copy Markdown
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 55. Check the log or trigger a new build to see more.

Comment thread include/bout/boundary_iterator.hxx
Comment thread include/bout/boundary_iterator.hxx
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx Outdated
Comment thread include/bout/boundary_iterator.hxx
Comment thread include/bout/parallel_boundary_region.hxx
Comment thread include/bout/parallel_boundary_region.hxx
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Comment thread include/bout/parallel_boundary_region.hxx Outdated
Makes it easier to reuse for other code
Mimicks the parallel case, to write FCI independent code
The boundary region does not match what is done for the parallel case,
thus porting it to a iterator does not work.
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.
BoundaryRegionIter has been delted
This allows to extend the boundary code to place the boundary further
away from the boundary.
This is more general and takes the offset() into account, and thus works
for cases where the boundary is between the first and second guard cell
Previously only the first boundary point was set
Taken from hermes-3, adopted for higher order
This allows to write code for FCI and non-FCI using templates.
Directly iterate over the points
Copy link
Copy Markdown
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 79. Check the log or trigger a new build to see more.

#include "bout/assert.hxx"
#include "bout/bout_types.hxx"
#include "bout/build_defines.hxx"
#include "bout/field2d.hxx"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: included header build_defines.hxx is not used directly [misc-include-cleaner]

Suggested change
#include "bout/field2d.hxx"
#include "bout/field2d.hxx"

int y() const { return _y; }
int z() const { return _z; }

protected:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change
protected:
Additional context

include/bout/boundary_iterator.hxx:157: previously declared here

protected:
^

#include <functional>
#include <string>
#include <utility>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: included header utility is not used directly [misc-include-cleaner]

Suggested change

#include <utility>

#include <bout/bout_enum_class.hxx>
#include <bout/field2d.hxx>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: included header bout_enum_class.hxx is not used directly [misc-include-cleaner]

Suggested change
#include <bout/field2d.hxx>
#include <bout/field2d.hxx>

//BOUT_ENUM_CLASS(BoundaryFreeExtrapolation, limited, exponential, linear);

template <typename impl>
class BoundaryRegionIterBase { /// get the index at the last point in domain
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: the implicit default constructor of the CRTP is publicly accessible; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]

Suggested change
class BoundaryRegionIterBase { /// get the index at the last point in domain
class BoundaryRegionIterBase {
BoundaryRegionIterBase() = default;
/// get the index at the last point in domain

include/bout/boundary_region_iter.hxx:362:

- ;
+ ;friend impl;

private:
// TODO(dave) make non-const?
const BoundaryRegionFCI* region;
size_t pos{0};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

include/bout/boundary_region_iter.hxx:2:

- #include <cstdint>
+ #include <cstddef>
+ #include <cstdint>

BoundaryRegionIterFCI(const BoundaryRegionFCI* reg, bool isstart)
: region(reg), pos(isstart ? 0 : reg->bndry_points.size()) {}
void setValid(char valid) {
const_cast<BoundaryRegionFCI*>(region)->bndry_points[pos].valid = valid;
Copy link
Copy Markdown
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 const_cast to remove const qualifier [cppcoreguidelines-pro-type-const-cast]

{
      ^

Comment thread include/bout/boundary_region_iter.hxx Outdated
ASSERT3(_valid() > -off - 2);
}
auto _off = _offset() - off * region->_dir;
return f.ynext(_off)[ind().yp(_off)];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f.ynext(_off)[ind().yp(_off)];
}
;()

Comment thread include/bout/boundary_region_iter.hxx Outdated
ASSERT3(_valid() > -off - 2);
}
auto _off = _offset() - off * region->_dir;
return f.ynext(_off)[ind().yp(_off)];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f.ynext(_off)[ind().yp(_off)];
}
;()

Comment thread include/bout/boundary_region_iter.hxx Outdated
ASSERT3(_valid() > -off - 2);
}
auto _off = _offset() - off * region->_dir;
return f.ynext(_off)[ind().yp(_off)];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f.ynext(_off)[ind().yp(_off)];
}
;()

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

//BOUT_ENUM_CLASS(BoundaryFreeExtrapolation, limited, exponential, linear);

template <typename impl>
class BoundaryRegionIterBase { /// get the index at the last point in domain
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: the implicit default constructor of the CRTP is publicly accessible; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]

Suggested change
class BoundaryRegionIterBase { /// get the index at the last point in domain
class BoundaryRegionIterBase {
BoundaryRegionIterBase() = default;
/// get the index at the last point in domain

include/bout/boundary_region_iter.hxx:369:

- ;
+ ;friend impl;

BoutReal extrapolate_boundary_free(const Field3D& f,
BoundaryFreeExtrapolation mode) const {
const auto fac = valid() > 0 ? limitFreeScale(prev(f), current(f), mode)
: (mode == BoundaryFreeExtrapolation::linear ? 0 : 1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

)
                                      ^
Additional context

include/bout/boundary_region_iter.hxx:251: parent conditional operator here

{
                       ^

}

void set_free(Field3D& f, BoundaryFreeExtrapolation mode) const {
int fac;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

include/bout/boundary_region_iter.hxx:343:

- ;
+  = 0;

Comment thread include/bout/boundary_region_iter.hxx Outdated
ASSERT3(_valid() > -off - 2);
}
auto _off = _offset() - off * region->_dir;
return f.ynext(_off)[ind().yp(_off)];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f.ynext(_off)[ind().yp(_off)];
}
;()

Comment thread include/bout/boundary_region_iter.hxx Outdated
ASSERT3(valid() > -off - 2);
}
auto _off = _offset() + off * region->_dir;
return f(_off, ind().yp(_off));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f(_off, ind().yp(_off));
}
;()

BoundaryOpPar() = default;
BoundaryOpPar(BoundaryRegionPar* region, std::shared_ptr<FieldGenerator> value)
BoundaryOpPar(bout::boundary::BoundaryRegionFCI* region,
std::shared_ptr<FieldGenerator> value)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "FieldGenerator" is directly included [misc-include-cleaner]

include/bout/parallel_boundary_op.hxx:10:

- #include "bout/unused.hxx"
+ #include "bout/sys/expressionparser.hxx"
+ #include "bout/unused.hxx"

BoundaryOpPar(bout::boundary::BoundaryRegionX* region, Field3D* value)
: bndryX(region), field_values(value), value_type(ValueType::FIELD) {}
BoundaryOpPar(bout::boundary::BoundaryRegionX* region, BoutReal value)
: bndryX(region), real_value(value), value_type(ValueType::REAL) {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: member initializer for 'value_type' is redundant [cppcoreguidelines-use-default-member-init]

Suggested change
: bndryX(region), real_value(value), value_type(ValueType::REAL) {}
: bndryX(region), real_value(value), {}

BoundaryOpPar(bout::boundary::BoundaryRegionX* region, BoutReal value)
: bndryX(region), real_value(value), value_type(ValueType::REAL) {}
BoundaryOpPar(bout::boundary::BoundaryRegionX* region)
: bndryX(region), real_value(0.), value_type(ValueType::REAL) {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: member initializer for 'real_value' is redundant [cppcoreguidelines-use-default-member-init]

Suggested change
: bndryX(region), real_value(0.), value_type(ValueType::REAL) {}
: bndryX(region), , value_type(ValueType::REAL) {}

BoundaryOpPar(bout::boundary::BoundaryRegionX* region, BoutReal value)
: bndryX(region), real_value(value), value_type(ValueType::REAL) {}
BoundaryOpPar(bout::boundary::BoundaryRegionX* region)
: bndryX(region), real_value(0.), value_type(ValueType::REAL) {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: member initializer for 'value_type' is redundant [cppcoreguidelines-use-default-member-init]

Suggested change
: bndryX(region), real_value(0.), value_type(ValueType::REAL) {}
: bndryX(region), real_value(0.), {}

BoundaryOpPar(BoundaryOpPar* region, Field3D* value)
: bndry(region->bndry), field_values(value), value_type(ValueType::FIELD) {}
BoundaryOpPar(BoundaryOpPar* region, BoutReal value)
: bndry(region->bndry), real_value(value), value_type(ValueType::REAL) {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: member initializer for 'value_type' is redundant [cppcoreguidelines-use-default-member-init]

Suggested change
: bndry(region->bndry), real_value(value), value_type(ValueType::REAL) {}
: bndry(region->bndry), real_value(value), {}

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

//BOUT_ENUM_CLASS(BoundaryFreeExtrapolation, limited, exponential, linear);

template <typename impl>
class BoundaryRegionIterBase { /// get the index at the last point in domain
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: the implicit default constructor of the CRTP is publicly accessible; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]

Suggested change
class BoundaryRegionIterBase { /// get the index at the last point in domain
class BoundaryRegionIterBase {
BoundaryRegionIterBase() = default;
/// get the index at the last point in domain

include/bout/boundary_region_iter.hxx:374:

- ;
+ ;friend impl;

BoutReal extrapolate_boundary_free(const Field3D& f,
BoundaryFreeExtrapolation mode) const {
const auto fac = valid() > 0 ? limitFreeScale(prev(f), current(f), mode)
: (mode == BoundaryFreeExtrapolation::linear ? 0 : 1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

)
                                      ^
Additional context

include/bout/boundary_region_iter.hxx:256: parent conditional operator here

{
                       ^

}

void set_free(Field3D& f, BoundaryFreeExtrapolation mode) const {
int fac;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

include/bout/boundary_region_iter.hxx:348:

- ;
+  = 0;

ASSERT3(_valid() > -off - 2);
}
auto _off = _offset() - off * region->_dir;
return f.ynext(_off)[_ind().yp(_off)];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f.ynext(_off)[_ind().yp(_off)];
}
;()

ASSERT3(_valid() > -off - 2);
}
auto _off = _offset() - off * region->_dir;
return f.ynext(_off)[_ind().yp(_off)];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
return f.ynext(_off)[_ind().yp(_off)];
}
;()

}

BoundaryOpPar* clone(bout::boundary::BoundaryRegionX* region, Field3D* f) override {
return new T(region, f);
Copy link
Copy Markdown
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);
    ^

if (!args.empty()) {
try {
real_value = stringToReal(args.front());
return new T(region, real_value);
Copy link
Copy Markdown
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, real_value);
        ^

}

BoundaryOpPar* clone(bout::boundary::BoundaryRegionY* region, Field3D* f) override {
return new T(region, f);
Copy link
Copy Markdown
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);
    ^

const int dir;
BoutReal extrapolate_sheath_free(const Field3D& f, SheathLimitMode mode) const {
const auto fac = valid() > 0 ? limitFreeScale(yprev(f), ythis(f), mode)
: (mode == SheathLimitMode::linear_free ? 0 : 1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

                                 : (mode == SheathLimitMode::linear_free ? 0 : 1);
                                    ^
Additional context

include/bout/parallel_boundary_region.hxx:301: parent conditional operator here

    const auto fac = valid() > 0 ? limitFreeScale(yprev(f), ythis(f), mode)
                     ^


void set_free(Field3D& f, SheathLimitMode mode) const {
const auto fac = valid() > 0 ? limitFreeScale(yprev(f), ythis(f), mode)
: (mode == SheathLimitMode::linear_free ? 0 : 1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

                                 : (mode == SheathLimitMode::linear_free ? 0 : 1);
                                    ^
Additional context

include/bout/parallel_boundary_region.hxx:309: parent conditional operator here

    const auto fac = valid() > 0 ? limitFreeScale(yprev(f), ythis(f), mode)
                     ^

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.

3 participants