-
Notifications
You must be signed in to change notification settings - Fork 104
Field3DParallel + more FCI changes #3197
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
base: next
Are you sure you want to change the base?
Conversation
Previously only the first boundary point was set
For FCI we need to be able to access "random" data from the adjacent slices. If they are split in x-direction, this requires some tricky communication pattern. It can be used like this: ``` // Create object GlobalField3DAccess fci_comm(thismesh); // let it know what data points will be required: // where IndG3D is an index in the global field, which would be the // normal Ind3D if there would be only one proc. fci_comm.get(IndG3D(i, ny, nz)); // If all index have been added, the communication pattern will be // established. This has to be called by all processors in parallel fci_comm.setup() // Once the data for a given field is needed, it needs to be // communicated: GlobalField3DAccessInstance global_data = fci_comm.communicate(f3d); // and can be accessed like this BoutReal data = global_data[IndG3D(i, ny, nz)]; // ny and nz in the IndG3D are always optional. ```
If they are two instances of the same template, this allows to have an if in the inner loop that can be optimised out.
This reverts commit d7d7c91.
We have the info, so might as well store it
Taken from hermes-3, adopted for higher order
lower_bound takes into account the data is sorted
Ensures we all ways check for monotonicity
Tags were different for sender and receiver
Otherwise mpi might wait for the wrong request.
There was a problem hiding this 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 219. Check the log or trigger a new build to see more.
| ******************************************************************/ | ||
|
|
||
| #define PVODE_BAND_ELEM(A,i,j) ((A->data)[j][i-j+(A->smu)]) | ||
| #define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'PVODE_BAND_ELEM' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)])
^| ******************************************************************/ | ||
|
|
||
| #define PVODE_BAND_COL(A,j) (((A->data)[j])+(A->smu)) | ||
| #define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
^| ******************************************************************/ | ||
|
|
||
| #define BAND_COL(A, j) (((A->data)[j]) + (A->smu)) | ||
| #define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
^| #include <algorithm> | ||
| #include <functional> | ||
|
|
||
| class BoundaryRegionIter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: class 'BoundaryRegionIter' 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 BoundaryRegionIter {
^| inline T toFieldAligned(const T& f, const std::string& region = "RGN_ALL") { | ||
| static_assert(bout::utils::is_Field_v<T>, "toFieldAligned only works on Fields"); | ||
| ASSERT3(f.getCoordinates() != nullptr); | ||
| return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member access into incomplete type 'Coordinates' [clang-diagnostic-error]
return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region);
^Additional context
include/bout/index_derivs_interface.hxx:218: in instantiation of function template specialization 'toFieldAligned' requested here
const T f_aligned = is_unaligned ? toFieldAligned(f, "RGN_NOX") : f;
^include/bout/index_derivs_interface.hxx:227: in instantiation of function template specialization 'bout::derivatives::index::DDY' requested here
return DDY(f.asField3D(), outloc, method, region);
^include/bout/field_data.hxx:42: forward declaration of 'Coordinates'
class Coordinates;
^|
|
||
| int tracking_state{0}; | ||
| Options* tracking{nullptr}; | ||
| std::string selfname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'selfname' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::string selfname;
^| explicit Field3DParallel(Types... args) : Field3D(std::move(args)...) { | ||
| ensureFieldAligned(); | ||
| } | ||
| Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
| Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } | |
| Field3DParallel(const Field3D& f) : Field3D(f) { ensureFieldAligned(); } |
| ensureFieldAligned(); | ||
| } | ||
| Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } | ||
| Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
| Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } | |
| Field3DParallel(const Field2D& f) : Field3D(f) { ensureFieldAligned(); } |
| }; | ||
|
|
||
| Field3DParallel Field3D::asField3DParallel() { return Field3DParallel(*this); } | ||
| const Field3DParallel Field3D::asField3DParallel() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: return type 'const Field3DParallel' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]
include/bout/field3d.hxx:533:
- inline const Field3DParallel asField3DParallel() const;
+ inline Field3DParallel asField3DParallel() const;| const Field3DParallel Field3D::asField3DParallel() const { | |
| Field3DParallel Field3D::asField3DParallel() const { |
|
|
||
| inline Field3DParallel | ||
| filledFrom(const Field3DParallel& f, | ||
| std::function<BoutReal(int yoffset, Ind3D index)> func) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: the parameter 'func' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
| std::function<BoutReal(int yoffset, Ind3D index)> func) { | |
| const std::function<BoutReal(int yoffset, Ind3D index)>& func) { |
There was a problem hiding this 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 219. Check the log or trigger a new build to see more.
| ******************************************************************/ | ||
|
|
||
| #define PVODE_BAND_ELEM(A,i,j) ((A->data)[j][i-j+(A->smu)]) | ||
| #define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'PVODE_BAND_ELEM' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define PVODE_BAND_ELEM(A, i, j) (((A)->data)[j][(i) - (j) + ((A)->smu)])
^| ******************************************************************/ | ||
|
|
||
| #define PVODE_BAND_COL(A,j) (((A->data)[j])+(A->smu)) | ||
| #define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
^| ******************************************************************/ | ||
|
|
||
| #define BAND_COL(A, j) (((A->data)[j]) + (A->smu)) | ||
| #define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'PVODE_BAND_COL' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define PVODE_BAND_COL(A, j) ((((A)->data)[j]) + ((A)->smu))
^| #include <algorithm> | ||
| #include <functional> | ||
|
|
||
| class BoundaryRegionIter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: class 'BoundaryRegionIter' 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 BoundaryRegionIter {
^| inline T toFieldAligned(const T& f, const std::string& region = "RGN_ALL") { | ||
| static_assert(bout::utils::is_Field_v<T>, "toFieldAligned only works on Fields"); | ||
| ASSERT3(f.getCoordinates() != nullptr); | ||
| return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member access into incomplete type 'Coordinates' [clang-diagnostic-error]
return f.getCoordinates()->getParallelTransform().toFieldAligned(f, region);
^Additional context
include/bout/index_derivs_interface.hxx:218: in instantiation of function template specialization 'toFieldAligned' requested here
const T f_aligned = is_unaligned ? toFieldAligned(f, "RGN_NOX") : f;
^include/bout/index_derivs_interface.hxx:227: in instantiation of function template specialization 'bout::derivatives::index::DDY' requested here
return DDY(f.asField3D(), outloc, method, region);
^include/bout/field_data.hxx:42: forward declaration of 'Coordinates'
class Coordinates;
^|
|
||
| int tracking_state{0}; | ||
| Options* tracking{nullptr}; | ||
| std::string selfname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'selfname' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::string selfname;
^| explicit Field3DParallel(Types... args) : Field3D(std::move(args)...) { | ||
| ensureFieldAligned(); | ||
| } | ||
| Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
| Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } | |
| Field3DParallel(const Field3D& f) : Field3D(f) { ensureFieldAligned(); } |
| ensureFieldAligned(); | ||
| } | ||
| Field3DParallel(const Field3D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } | ||
| Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: std::move of the const variable 'f' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
| Field3DParallel(const Field2D& f) : Field3D(std::move(f)) { ensureFieldAligned(); } | |
| Field3DParallel(const Field2D& f) : Field3D(f) { ensureFieldAligned(); } |
| }; | ||
|
|
||
| Field3DParallel Field3D::asField3DParallel() { return Field3DParallel(*this); } | ||
| const Field3DParallel Field3D::asField3DParallel() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: return type 'const Field3DParallel' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]
include/bout/field3d.hxx:533:
- inline const Field3DParallel asField3DParallel() const;
+ inline Field3DParallel asField3DParallel() const;| const Field3DParallel Field3D::asField3DParallel() const { | |
| Field3DParallel Field3D::asField3DParallel() const { |
|
|
||
| inline Field3DParallel | ||
| filledFrom(const Field3DParallel& f, | ||
| std::function<BoutReal(int yoffset, Ind3D index)> func) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: the parameter 'func' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
| std::function<BoutReal(int yoffset, Ind3D index)> func) { | |
| const std::function<BoutReal(int yoffset, Ind3D index)>& func) { |
|
Please can you split this into one PR per change? It's 100 files and 5k lines of changes, it's going to be very difficult to review. GitHub will only show a single file at once. |
|
Sure! |
Information like perpendicular J, Bxyz and g_22 are only known to the grid generator, but are needed for more correct parallel derivatives
J on the parallel slices is not accurate.
This avoids mostly the need for annotations using BOUT_USE_TRACK. setName could thus be dropped.
Add unit test as well as fix
There was a problem hiding this 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 288. Check the log or trigger a new build to see more.
| const FieldMetric& Jxz() const; | ||
|
|
||
| private: | ||
| mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::optional" is directly included [misc-include-cleaner]
include/bout/coordinates.hxx:40:
+ #include <optional>|
|
||
| private: | ||
| mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh; | ||
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: declaration uses identifier '_Jxz_centre', which is a reserved identifier [bugprone-reserved-identifier]
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre; | |
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, Jxz_centre; |
|
|
||
| private: | ||
| mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh; | ||
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: declaration uses identifier '_Jxz_yhigh', which is a reserved identifier [bugprone-reserved-identifier]
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre; | |
| mutable std::optional<FieldMetric> _Jxz_ylow, Jxz_yhigh, _Jxz_centre; |
|
|
||
| private: | ||
| mutable std::optional<FieldMetric> _g_22_ylow, _g_22_yhigh; | ||
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: declaration uses identifier '_Jxz_ylow', which is a reserved identifier [bugprone-reserved-identifier]
| mutable std::optional<FieldMetric> _Jxz_ylow, _Jxz_yhigh, _Jxz_centre; | |
| mutable std::optional<FieldMetric> Jxz_ylow, _Jxz_yhigh, _Jxz_centre; |
|
|
||
| template <typename T> | ||
| auto& getStore() { | ||
| if constexpr (std::is_same<T, Field3DParallel>::value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "Field3DParallel" is directly included [misc-include-cleaner]
include/bout/deriv_store.hxx:37:
- #include <bout/scorepwrapper.hxx>
+ #include "bout/field3d.hxx"
+ #include <bout/scorepwrapper.hxx>| #include <bout/mesh.hxx> | ||
| #if CHECK > 0 | ||
| #include <bout/output_bout_types.hxx> | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header output_bout_types.hxx is not used directly [misc-include-cleaner]
| #endif | |
| #endif |
| // Left and right cell face values | ||
| BoutReal L, R; | ||
| /// Cell centre values | ||
| BoutReal c; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "BoutReal" is directly included [misc-include-cleaner]
include/bout/fv_ops.hxx:7:
- #include "bout/build_defines.hxx"
+ #include "bout/bout_types.hxx"
+ #include "bout/build_defines.hxx"| BoutReal m; | ||
| BoutReal p; | ||
| BoutReal mm = BoutNaN; | ||
| BoutReal pp = BoutNaN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "BoutNaN" is directly included [misc-include-cleaner]
BoutNaN;
^| BoutReal flux = NAN; | ||
|
|
||
| if (mesh->lastY(i) && (j == mesh->yend) && !mesh->periodicY(i)) { | ||
| if (is_last_y && (j == mesh->yend) && !is_periodic_y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "NAN" is directly included [misc-include-cleaner]
Real flux = NAN;
^| const auto& v_down = v_in.ydown(); | ||
|
|
||
| Field3D result{emptyFrom(f_in)}; | ||
| BOUT_FOR(i, f_in.getRegion("RGN_NOBNDRY")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "emptyFrom" is directly included [misc-include-cleaner]
emptyFrom(f_in)};
^
Here a summary of the changes:
Field3DParallel
A Field3DParallel is essentially a Field3D, that preserves the parallel slices.
This allows to avoid unnecessary computation of of parallel fields, that are not needed.
It also ensures parallel slices are present (for FCI) when later parallel derivatives are computed.
So any function that will call
DDYlater on, can change the signature toField3DParallelto declare that parallel slices need to be present for FCI.New Y-Boundary Operators
Custom sheath boundary conditions can now be implemented using an abstraction. The documentation is on RTD
That allows having one code path for FCI and FA, for upper and lower sheath BC, all in one place.
Various small fixups
loadParallelMetrics
For FCI the parallel slices of the metric components are loaded, as they are sometimes needed.
This makes FCI essentially 3D only.
There is now also the option to say a field is not allow to compute the parallel slices, to avoid overwriting them:
allowCalcParallelSlices.Tracking on failure
Only if the simulation fails to evolve (Currently euler and pvode only):
The different components of timederivate are dumped to a
BOUT.debug.*.ncwhich can be handy for figuring out which term is causing the instability. No more having to re-run the simulation with some terms disabled to debug such issues. This causes (an unkown) memory overhead - but ONLY if the solver has failed already.More names of parallel boundary region + cleanup of code
Support things like
bndry_par_xinorbndry_par_yup.Add monotonichermitespline for all cases
Merged into hermitespline and works also in parallel.
Support several parallel slices for FCI
MYG=2is now supported.