-
Notifications
You must be signed in to change notification settings - Fork 11
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
Unify derivs #131
base: main
Are you sure you want to change the base?
Unify derivs #131
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
@@ -1028,6 +1028,7 @@ void carpetx_openpmd_t::OutputOpenPMD(const cGH *const cctkGH, | |||
series->setIterationEncoding(iterationEncoding); | |||
|
|||
{ | |||
// Does not work in containers |
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.
This change is unrelated.
Derivs/src/derivs.cxx
Outdated
|
||
// Tile-based multi-dimensional derivative operators | ||
|
||
template <int CI, int CJ, int CK, typename T> |
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.
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.
Derivs/src/derivs.hxx
Outdated
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The include guard needs to correspond to the file name
|
||
using Arith::pow2; | ||
|
||
template <typename T> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All functions should be put into a namespace.
dx; | ||
} | ||
|
||
using Loop::GF3D2; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
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.
Rename gf_
to gf
.
using Loop::GF3D5index; | ||
using Arith::smat; | ||
using Loop::PointDesc; | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
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.
Use dim
instead of 3
.
using Arith::pow2; | ||
|
||
template <typename T> | ||
inline ARITH_INLINE ARITH_DEVICE ARITH_HOST T pow3(const T &x) { |
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.
Is pow3
used?
|
||
using Arith::pown; | ||
|
||
template <int dir, typename T, int D> |
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.
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?
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.
I added comments requesting changes.
Steve, Erik: any progress on this one? |
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