Functions in ApparentHorizons/Tags.cpp are now free functions.

[markscheel]

Proposed changes

This is some code cleanup that I wanted to finish before doing #3561

Previously ApparentHorizons/Tags.cpp defined many functions inside of
ComputeItems. These should have been free functions instead of
ComputeItems.

These functions have now been moved from Tags.cpp to
StrahlkorperFunctions.hpp where they are now free functions.
The ComputeTags in ApparentHorizons/Tags.hpp now point to those
free functions.

For the most part, code has simply been moved from Tags.cpp to
StrahlkorperFunctions.cpp, but there were a few other changes:

• Some of the new free functions use fewer memory allocations
  than the corresponding ComputeItems did. This is accomplished
  by storing temporaries in components of the final result, and
  computing everything in a carefully chosen order so that memory
  is re-used.
• The free functions all have versions that return a value and
  versions that take and fill a not_null pointer.
• The functions that take derivatives of the radius have been
  renamed and generalized to take derivatives of an arbitrary scalar
  on the Strahlkorper.
• LaplacianRadius now returns a Scalar instead of a
  DataVector.
• Some of the documentation has been clarified.
• Tests have been added for the free functions.
• There is still a single ComputeItem remaining in Tags.cpp, for
  a function that doesn't need to be a free function
  (because it's just a member function of Strahlkorper).

Upgrade instructions

The only interface difference (other than the existence of new functions) is that ah::Tags::LaplacianRadius is now a Scalar<DataVector> instead of a DataVector.

Code review checklist

• The code is documented and the documentation renders correctly. Run
  make doc to generate the documentation locally into BUILD_DIR/docs/html.
  Then open index.html.
• The code follows the stylistic and code quality guidelines listed in the
  code review guide.
• The PR lists upgrade instructions and is labeled bugfix or
  new feature if appropriate.

Further comments

[nilsvu]

Is there a reason why you have chosen upper / lower indices for theta_phi, rhat, etc? It probably doesn't matter much, unless you want to use them with tensor expressions at some point. If there's a design decision behind this though, then please add some documentation, e.g. to a namespace or to these function dox.

[nilsvu]

no need to repeat the dox, that's what /// @{ is for. Applies to all functions in this namespace.

[nilsvu]

(optional) I suggest to structure these docs with Doxygen commands:

 * \param radius The radius as a function of angle, as returned by ...
 * \param r_hat The Euclidean radial unit vector ...

This would apply to all functions in this namespace, if you choose to do it.

[nilsvu]

👍 squash

[nilsvu]

@markscheel Doxygen wants you to document all params now, which I think is a good idea :)

[markscheel]

It appears that Doxygen does not like the combination of \param and having two functions of different signatures in /// @{, because it requires all \params to be documented. For example

/// @(
/*!
 * Dox here
 */
template <typename Fr>
Scalar<DataVector> radius(const Strahlkorper<Fr>& strahlkorper);

template <typename Fr>
void radius(const gsl::not_null<Scalar<DataVector>*> result,
            const Strahlkorper<Fr>& strahlkorper);
/// @}

If the documentation contains \param strahlkorper, but not \param result, doxygen dies because result is undocumented in the second function and it tells me to document it. If the documentation contains both \param strahlkorper and \param result, then it dies because it can't find a param called result in the first function.

So which do you prefer?

• Eliminate the /// @{ and duplicate the dox twice, once for each function
• Keep the /// @{ and duplicate the dox twice, once for each function
• Eliminate the /// @{ and add dox only for one of the functions
• Eliminate \param
• Something else?

[nilsvu]

@markscheel I believe you can do this (see Doxygen member groups):

/*!
 * \name Jacobian
 * Description, math and stuff.
 */
///@{
/*!
 * Optional details for first overload
 *
 * \param theta_phi Parameter description
 */
void jacobian(...);
/*!
 * Optional details for second overload
 *
 * \param theta_phi Parameter description
 * \param other_param Other parameter description
 */
void jacobian(...);
/// @}

What do you think?

[nilsvu]

LGTM, docs render fine 👍