Conversation
(1) Discard existing derivs.hxx and derivs.cxx (2) Merge z4c and weyl derivs.hxx (3) move epsdiss to Derivs
Conflicts: Derivs/src/derivs.hxx
CarpetX/src/io_openpmd.cxx
Outdated
| series->setIterationEncoding(iterationEncoding); | ||
|
|
||
| { | ||
| // Does not work in containers |
There was a problem hiding this comment.
This change is unrelated.
|
|
||
| // Tile-based multi-dimensional derivative operators | ||
|
|
||
| template <int CI, int CJ, int CK, typename T> |
There was a problem hiding this comment.
The tile-based derivative operators in thorn Derivs were implemented in C++ sources files (as opposed to header files) because they would not be inlined. There is no advantage to having them in a header file, and one disadvantage is that it increased build times.
|
|
||
| constexpr stencil<-2, +2, symmetric> deriv2_o4{12, {-1, +16, -30, +16, -1}}; | ||
|
|
||
| // Interpolate at i = 1/2 |
There was a problem hiding this comment.
These stencils are new capabilities in thorn Derivs that are not present in the thorns Z4c and Weyl. If you move the Z4c and Weyl code over, and delete the existing code in Derivs, then you are removing capabilities. The current implementation of thorn Derivs is a more "modern" design than those in Z4c or Weyl, which kind-of grew wild.
Derivs/src/derivs_spacetimex.hxx
Outdated
| @@ -0,0 +1,512 @@ | |||
| #ifndef CARPETX_DERIVS_DERIVS_HXX | |||
There was a problem hiding this comment.
The include guard needs to correspond to the file name
|
|
||
| using Arith::pow2; | ||
|
|
||
| template <typename T> |
There was a problem hiding this comment.
This function should not be in the global name space.
|
|
||
| template <typename T> | ||
| inline ARITH_INLINE ARITH_DEVICE ARITH_HOST simd<T> | ||
| deriv1d(const simdl<T> &mask, const T *restrict const var, const ptrdiff_t di, |
There was a problem hiding this comment.
All functions should be put into a namespace.
| dx; | ||
| } | ||
|
|
||
| using Loop::GF3D2; |
There was a problem hiding this comment.
It's weird to sprinkle the using declarations across the file. They should either be inside functions, or should be collected near the top similar to #include statements.
| inline ARITH_INLINE ARITH_DEVICE ARITH_HOST simd<T> | ||
| deriv(const simdl<T> &mask, const GF3D2<const T> &gf_, const vect<int, dim> &I, | ||
| const vec<T, D> &dx) { | ||
| static_assert(dir >= 0 && dir < D, ""); |
There was a problem hiding this comment.
The empty string is not necessary in C++17.
|
|
||
| template <int dir, typename T, int D> | ||
| inline ARITH_INLINE ARITH_DEVICE ARITH_HOST simd<T> | ||
| deriv(const simdl<T> &mask, const GF3D2<const T> &gf_, const vect<int, dim> &I, |
| using Loop::GF3D5index; | ||
| using Arith::smat; | ||
| using Loop::PointDesc; | ||
|
|
There was a problem hiding this comment.
Reformat the file with clang-format.
|
|
||
| template <typename T> | ||
| CCTK_ATTRIBUTE_NOINLINE void | ||
| calc_derivs(const cGH *restrict const cctkGH, const GF3D2<const T> &gf1, |
There was a problem hiding this comment.
The input variables are called ...1, and the output variables ...0. That's a weird scheme. Maybe call the input quantities ...0, and use no suffix for the output quantities?
| }); | ||
| } | ||
|
|
||
| using std::enable_if_t; |
There was a problem hiding this comment.
It's customary in C++ to explicitly write the std:: prefix when using standard library types.
| const vec<GF3D2<const T>, dim> &gf0_, | ||
| const vec<GF3D5<T>, dim> &gf_, | ||
| const GF3D5layout &layout) { | ||
| for (int a = 0; a < 3; ++a) |
| using Arith::pow2; | ||
|
|
||
| template <typename T> | ||
| inline ARITH_INLINE ARITH_DEVICE ARITH_HOST T pow3(const T &x) { |
|
|
||
| using Arith::pown; | ||
|
|
||
| template <int dir, typename T, int D> |
There was a problem hiding this comment.
The functions above assume a fixed number of dimensions. This function is now generic (with a D template parameter). It also mixes dim and D. Is D necessary?
eschnett
left a comment
eschnett
left a comment
There was a problem hiding this comment.
I added comments requesting changes.
|
Steve, Erik: any progress on this one? |
3 participants
Unify derivatives between CarpetX and SpacetimeX.
(1) Discard existing derivs.hxx and derivs.cxx
(2) Merge z4c and weyl derivs.hxx
(3) move epsdiss to Derivs