[3D metric] Ensure boundaries are valid #2326
Conversation
Uses CVodeSetConstraints function to allow constraints to be applied to variables to be: none (default, no constraint), positive, non_negative, negative, or non_positive.
The Coordinates' metric elements can hold shared_ptrs that result in a cyclic dependency between the Fields and the Coordinates, with the result that some unit tests have memory leaks. One place the issue can arise is when multiplying fields by metric elements with `CHECK > 0`. Then, `ASSERT1_FIELDS_COMPATIBLE` checks that both fields share a `Coordinates` -- but this has the side effect of initialising the `shared_ptr` in the metric elements. This means that when the `Mesh` gets destroyed, the `coords_map` starts to destroy its contents, but because the `Coordinates` metric elements members have references to the `Coordinates`, the `Coordinates` in `coords_map` doesn't get destroyed. The end result is that the `Coordinates` gets leaked. The fix here is to use a `weak_ptr` in `Field`. This only gets a reference to the `Coordinates` when needed.
[skip ci]
Reduces code duplication.
CVODE constraints and max_noinlinear_iterations options
CMake: Build documentation
* next: (28 commits) Move constraint creation to utility method CMake: Default to not building the docs CMake: Be explicit that docs aren't built in `all` CMake: Fix typo Make method parameter names consistent (clang-tidy fix) Check version of SUNDIALS is new enough for constraints Option to set positivity constraints in CVODE Option to change max nonlinear iterations in CVODE GHA: Don't build the docs for other Github actions either GHA: Don't build docs on github CMake: Build documentation [skip ci] Apply black changes Update installation suggestions for Marconi/gnu fix order of date and time Fix compilation Faster recompile Remove checks for old behaviour of bool conversion Use lowercase() from utils.hxx Make conversion of Option to bool stricter Add brackets to previous commit ...
…aries Add Mesh method to get all possible boundary regions
Delete integrated test of FieldFactory: replaced by unit tests
* next: (349 commits) Delete integrated test of FieldFactory: replaced by unit tests Convert ersatz MMS tests to use library MMS features Disable tests by default if building as part of another project CMake rename PACKAGE_TESTS to BOUT_TESTS Add some docs for formatting `Options` to string Fix for conditionally used boundary condition inputs in 'all' Further improve test-invpar fix WithQuietOutput speedup test-invpar Update .gitignore Enable running tests in parallel Add test for invalid Options format string Fix more clang-tidy suggestions Fix clang-tidy suggestions Replace OptionINI::writeSection with fmt::format Print docstring and type info for unused inputs Fix `Options` formatting to handle values correctly Add more format specs for `Options`: key-only and source attribute Add `fmt::formatter` for `Options` Restrict Options implicit cast operator to only types in variant ...
Now stops immediately if option was `MAYBE`, and FATAL_ERROR if option was turned on
CMake: Build Python API
Fixing up 6field-simple example in next
Bump boutdata, boututils versions
CI: Apply black to python files in src
|
The python interface doesn't currently work with 3D + out of source builds. I am tempted to fix this based on #2351 ... so disable that in CI for now. |
dschwoerer
force-pushed
the
coords-boundary
branch
from
e1a2e5d
to
59fa2cf
Compare
dschwoerer
force-pushed
the
coords-boundary
branch
from
59fa2cf
to
867e303
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // The maxregionblocksize to use when creating the default regions. | ||
| // Can be set in the input file and the global default is set by, | ||
| // MAXREGIONBLOCKSIZE in include/bout/region.hxx | ||
| int maxregionblocksize{MAXREGIONBLOCKSIZE}; |
There was a problem hiding this comment.
warning: member variable maxregionblocksize has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
int maxregionblocksize{MAXREGIONBLOCKSIZE};
^|
|
||
| /// Enable staggered grids (Centre, Lower). Otherwise all vars are | ||
| /// cell centred (default). | ||
| bool StaggerGrids{false}; |
There was a problem hiding this comment.
warning: member variable StaggerGrids has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool StaggerGrids{false};
^| // Parses format specifications of the form ['c' | 'i']. | ||
| constexpr auto parse(format_parse_context& ctx) { | ||
| auto it = ctx.begin(), end = ctx.end(); | ||
| if (it != end && (*it == 'c' || *it == 'i')) { |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
auto it = ctx.begin(), end = ctx.end();
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| protected: | ||
| /// Grid information, etc. Owned by the simulation or global object | ||
| Mesh* fieldmesh{nullptr}; |
There was a problem hiding this comment.
warning: member variable fieldmesh has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
Mesh* fieldmesh{nullptr};
^| // Note: const char* version needed to avoid conversion to bool | ||
| template<> inline void Options::assign<>(const char *val, const std::string source) { _set(std::string(val), source, false);} | ||
| template<> inline void Options::assign<>(const char *val, std::string source) { _set(std::string(val), source, false);} |
There was a problem hiding this comment.
warning: parameter source is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
template<> inline void Options::assign<>(const char *val, std::string source) { _set(std::string(val), source, false);}
^
std::move( )| sendvec[kz] = x(kz, n_mpi); | ||
| } | ||
| MPI_Isend(&sendvec[0], nsys, MPI_DOUBLE_COMPLEX, myrank + 1, 100, comm, request + 1); | ||
| } |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
MPI_Isend(&sendvec[0], nsys, MPI_DOUBLE_COMPLEX, myrank + 1, 100, comm, request + 1);
^| } | ||
| if (xproc < nprocs - 1) { | ||
| MPI_Wait(request + 1, &status); | ||
| } |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
MPI_Wait(request + 1, &status);
^|
|
||
| namespace { | ||
| RegisterLaplace<LaplacePCR> registerlaplacepcr(LAPLACE_PCR); | ||
| } |
There was a problem hiding this comment.
warning: variable registerlaplacepcr defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]
RegisterLaplace<LaplacePCR> registerlaplacepcr(LAPLACE_PCR);
^| } | ||
|
|
||
| class LaplacePCR : public Laplacian { | ||
| public: |
There was a problem hiding this comment.
warning: class LaplacePCR defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class LaplacePCR : public Laplacian {
^| class LaplacePCR : public Laplacian { | ||
| public: | ||
| LaplacePCR(Options* opt = nullptr, const CELL_LOC loc = CELL_CENTRE, | ||
| Mesh* mesh_in = nullptr); |
There was a problem hiding this comment.
warning: parameter loc is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
LaplacePCR(Options* opt = nullptr, const CELL_LOC loc = CELL_CENTRE,
^~~~~~
| @@ -73,113 +77,42 @@ BoutMesh::~BoutMesh() { | |||
| clear_handles(); | |||
|
|
|||
| // Delete the boundary regions | |||
| for (const auto &bndry : boundary) | |||
| for (const auto& bndry : boundary) { | |||
| delete bndry; | |||
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked gsl::owner<>; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete bndry;
^
src/mesh/impls/bout/boutmesh.cxx:80:8: note: variable declared here
for (const auto& bndry : boundary) {
^| delete bndry; | ||
| for (const auto &bndry : par_boundary) | ||
| } | ||
| for (const auto& bndry : par_boundary) { | ||
| delete bndry; |
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked gsl::owner<>; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete bndry;
^
src/mesh/impls/bout/boutmesh.cxx:83:8: note: variable declared here
for (const auto& bndry : par_boundary) {
^| if (((yg > jyseps1_1) and (yg <= jyseps2_1)) | ||
| or ((yg > jyseps1_2) and (yg <= jyseps2_2))) { | ||
| // Core | ||
| boundary.push_back(new BoundaryRegionXIn("core", ystart, yend, this)); |
There was a problem hiding this comment.
warning: initializing non-owner argument of type std::vector<BoundaryRegion *, std::allocator<BoundaryRegion *> >::value_type && (aka BoundaryRegion *&&) with a newly created gsl::owner<> [cppcoreguidelines-owning-memory]
boundary.push_back(new BoundaryRegionXIn("core", ystart, yend, this));
^| boundary.push_back(new BoundaryRegionXIn("core", ystart, yend, this)); | ||
| } else { | ||
| // PF region | ||
| boundary.push_back(new BoundaryRegionXIn("pf", ystart, yend, this)); |
There was a problem hiding this comment.
warning: initializing non-owner argument of type std::vector<BoundaryRegion *, std::allocator<BoundaryRegion *> >::value_type && (aka BoundaryRegion *&&) with a newly created gsl::owner<> [cppcoreguidelines-owning-memory]
boundary.push_back(new BoundaryRegionXIn("pf", ystart, yend, this));
^| // Need boundaries in Y | ||
| if (PE_XIND == (NXPE - 1)) { | ||
| // Outer SOL | ||
| boundary.push_back(new BoundaryRegionXOut("sol", ystart, yend, this)); |
There was a problem hiding this comment.
warning: initializing non-owner argument of type std::vector<BoundaryRegion *, std::allocator<BoundaryRegion *> >::value_type && (aka BoundaryRegion *&&) with a newly created gsl::owner<> [cppcoreguidelines-owning-memory]
boundary.push_back(new BoundaryRegionXOut("sol", ystart, yend, this));
^| status = model->runConvective(t); | ||
| }else | ||
| status = (*phys_conv)(t); | ||
| status = model->runConvective(t); |
There was a problem hiding this comment.
warning: Value stored to status is never read [clang-analyzer-deadcode.DeadStores]
status = model->runConvective(t);
^
src/solver/solver.cxx:1249:5: note: Value stored to 'status' is never read| post_rhs(t); | ||
| save_derivs(tmp2.begin()); // Save time derivatives | ||
| for(BoutReal *t = tmp.begin(), *t2 = tmp2.begin(); t != tmp.end(); ++t, ++t2) | ||
| for (BoutReal *t = tmp.begin(), *t2 = tmp2.begin(); t != tmp.end(); ++t, ++t2) { |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
for (BoutReal *t = tmp.begin(), *t2 = tmp2.begin(); t != tmp.end(); ++t, ++t2) {
^| post_rhs(t); | ||
| save_derivs(tmp2.begin()); // Save time derivatives | ||
| for(BoutReal *t = tmp.begin(), *t2 = tmp2.begin(); t != tmp.end(); ++t, ++t2) | ||
| for (BoutReal *t = tmp.begin(), *t2 = tmp2.begin(); t != tmp.end(); ++t, ++t2) { |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
for (BoutReal *t = tmp.begin(), *t2 = tmp2.begin(); t != tmp.end(); ++t, ++t2) {
^| @@ -44,13 +44,20 @@ void OptionsReader::write(Options *options, const std::string& filename) { | |||
| } | |||
|
|
|||
| void OptionsReader::parseCommandLine(Options* options, int argc, char** argv) { | |||
| return parseCommandLine(options, std::vector<std::string>(argv, argv + argc)); | |||
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
return parseCommandLine(options, std::vector<std::string>(argv, argv + argc));
^| return parseCommandLine(options, std::vector<std::string>(argv, argv + argc)); | ||
| } | ||
|
|
||
| void OptionsReader::parseCommandLine(Options *options, const std::vector<std::string>& argv) { |
There was a problem hiding this comment.
warning: method parseCommandLine can be made static [readability-convert-member-functions-to-static]
void OptionsReader::parseCommandLine(Options *options, const std::vector<std::string>& argv) {
^
| } else { | ||
| output_error.write(" failure\n"); | ||
| } | ||
| return success; |
There was a problem hiding this comment.
warning: implicit conversion bool -> int [readability-implicit-bool-conversion]
return success;
^
static_cast<int>( )|
|
||
| int allpassed; | ||
| MPI_Allreduce(&passed, &allpassed, 1, MPI_INT, MPI_MIN, BoutComm::get()); | ||
| int main(int argc, char** argv) { |
There was a problem hiding this comment.
warning: an exception may be thrown in function main which should not throw exceptions [bugprone-exception-escape]
int main(int argc, char** argv) {
^| MPI_Barrier(BoutComm::get()); | ||
|
|
||
| BoutFinalise(); | ||
| return 0; | ||
| return !allsuccess; |
There was a problem hiding this comment.
warning: implicit conversion bool -> int [readability-implicit-bool-conversion]
return !allsuccess;
^
static_cast<int>( )
After communication we might have invalid points, this is fixed by
applying boundaries.