Multipatch support in NewRadX

[lucass-carneiro]

This PR adds Multipatch support to NewRadX.

This support is done in a way that no specific multipatch driver is required, i.e., it does not require CapyrX to function. It also assumes no particular patch structure or that a special radial coordinate is available. The new functionality is provided via an overloaded function of the original, and all features are opt-in.

A new parameter has been introduced, which allows one to use the BC in inner boundaries, if the patch system contains them.

[lucass-carneiro]

Ticket is here

[chcheng3]

This PR looks good to me, I only have a few minor suggestions. I also appreciate that besides adding Multipatch support, the PR also makes the kernel in NewRadX_Apply easier to read by providing inline functions for computing spatial derivatives.

[chcheng3]

Is there a use case where one would want to steer the parameter apply_inner_boundary?

[lucass-carneiro]

Personally, I can't foresee such use case at the moment, but it may be required by someone in the future.

[chcheng3]

Can you add descriptions for the new arguments introduced for the Multipatch version of NewRadX_Apply? Namely, I would like to know the what the quantities vcoordx, vJ_da_dx, etc represent. Where would they be provided? Would a thorn implementing a Multipatch infrastructure provide them, e.g. CapyrX?

[lucass-carneiro]

I've added new docstrings, following the format of the original function. Please take a look and let me know if you'd like anything changed.

[chcheng3]

I am getting a compiler warning that the calc_deriv function does not have a return statement at the end:

/projects/bcvu/chcheng3/ETK_CanudaX/Cactus/arrangements/SpacetimeX/NewRadX/src/newradx.cxx(79): warning #940-D: missing return statement at end of non-void function "NewRadX::calc_deriv<dir>(const Loop::PointDesc &, const Loop::GF3D2<const CCTK_REAL8> &) noexcept [with dir=0UL]"
  }
  ^
          detected during instantiation of "CCTK_REAL8 NewRadX::calc_deriv<dir>(const Loop::PointDesc &, const Loop::GF3D2<const CCTK_REAL8> &) noexcept [with dir=0UL]" at line 120

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

This looks harmless to me, but is there a way to address it?

[rhaas80]

you cannot assume that assert(0) actually stops the code (one can make an assert into a no-op by using the NDEBUG define). So this needs to also have an call to abort() or be replace by CCTK_ERROR() which is never returning and tells the compiler that this is the case.

[lucass-carneiro]

This is a host/device function. It cannot call CCTK_V* directly, if it is on the device correct?

In such cases, what I do is something like

#if !defined(__CUDACC__) && !defined(__HIP_PLATFORM_AMD__) &&                  \
    !defined(__HIP_PLATFORM_HCC__) && !defined(__INTEL_LLVM_COMPILER)
    CCTK_VERROR("...");
#else
    assert(false);
#endif

However we are back to the same assert(0) issue.

If we look at the surrounding code, we see

if (p.NI[dir] == 0) {
  ...
} else if (p.NI[dir] == +1) {
  ...
} else if (p.NI[dir] == -1) {
  ...
} else {
  assert(0);
}

This could prob be safely replaced by

if (p.NI[dir] == 0) {
  ...
} else if (p.NI[dir] > 0) {
  ...
} else {
  // if (p.NI[dir] == -1) code here
  ...
} 

If so, I can change it.

[rhaas80]

There's apparently amrex::Abort() which is good on GPUs as well:

https://github.com/AMReX-Codes/amrex/blob/9aaff4128aa54d8fe59c577bb91b58490ed4de72/Src/Base/AMReX_Array4.H#L1249