[ET Trac] [Einstein Toolkit] #2101: Add Lean_public and Proca thorns to the ET

[Einstein Toolkit]

#2101: Add Lean_public and Proca thorns to the ET
---------------------------------------+---------------------------------
  Reporter:  miguel.zilhao.nogueira@…  |      Owner:  Steven R. Brandt
      Type:  enhancement               |     Status:  assigned
  Priority:  major                     |  Milestone:  ET_2018_08
 Component:  EinsteinToolkit thorn     |    Version:  development version
Resolution:                            |   Keywords:  Lean_public
---------------------------------------+---------------------------------

Comment (by Peter Diener):

 Here is my review of the "Proca" suite of thorns that are proposed for
 inclusion in the Einstein Toolkit. Proca consists of ProcaBase,
 Proca_simpleID, TwoPunctures_KerrProca and ProcaEvolve and
 NPScalars_Proca. The code is nicely written, but mainly lack some tests
 and documentation.

  Questions, comments and recommendations regarding Proca overall:

 There are two tests in ProcaEvolve of which one uses Lean and the other
 ML_BSSN
 while otherwise appear to be the same. Whereas the Ai and Ei variables
 agree
 quite well, there seems to be significant differences between Aphi and
 Zeta.
 Is this expected? If so, why?

 In ProcaEvolve there is another parameter file in the test directory
 (ML_BSSN_Aphi_mu0.4_c100.1_r06_S0.0_4th.par) that does not have any
 associated
 output. If this was meant to be a test, output should be provided.
 Otherwise
 it should be moved to the example parameter file directory.

 NPScalars_Proca, Proca_simpleID and TwoPunctures_KerrProca have no tests
 of their own. Proca_simpleID is tested by the tests in ProcaEvolve, but
 NPScalars_Proca and TwoPunctures_KerrProca should definitely be covered by
 tests. You may also consider making a standalone test for Proca_simpleID.

 None of the thorns in the Proca arrangement have any documentation of
 their
 own. As there is a paper describing the formulation, it is okay to refer
 to the
 paper, but there should be some description of the variables and
 parameters
 used in the code and how they relate to the physics and equations.

 It would be nice if all he parameter files for the runs in the paper could
 be
 made available as examples of how to use the thorn. Currently there are
 only
 two example parameter files available and it's not immediately clear if
 they
 have been used for the paper. There could be a comment at the top of the
 parameterfiles indicating what section and/or figure in the paper they
 were
 used for.


  Questions, comments and recommendations specifically to ProcaBase:

 In parameter.ccl the comment to the value of the parameter mu says:
 "any non-negative number". However, the range allows for zero and in fact
 the
 default value is zero. Either the comment should be changed or the range
 and
 default value should be changed.

 Are the integer power for the fall off rate for the outgoing boundary
 conditions for the Proca variables not known? Or can they be different
 for different physical systems? If they are known constants, should they
 be parameters at all? Also could the power be different for different
 components of the E and A vectors. If not, there's no need to make these
 vector parameters.

 I propose to give the file (src/Basegrid.F90) with the symmetry
 registering
 routine a more descriptive name.

 It would be nice to have a proper (but short) thorn documentation to
 relate
 the variables with the paper.

  Questions, comments and recommendations specifically to Proca_simpleID:

 The Simple Initial Data solution in the paper is a spherical symmetric
 solution. The parameters for Proca_simpleID on the other hand seem to
 allow for a superposition of 2 such solutions separated by a non-zero
 distance. As the parameter for the separation can not be zero (imposed by
 the parameter range) the initial data thorn will thus never produce
 initial
 data that is spherically symmetric around the origin of the cactus grid.
 Would it make sense for the range of par_b to include zero in order to
 allow for that. This is something worth explaining in the (missing)
 documentation.


  Questions, comments and recommendations specifically to
 TwoPunctures_KerrProca:

 It looks like the code implements initial data with a Y_11 angular
 dependence
 in addition to Y_00 and Y_10 mentioned in the paper. This should be in the
 documentation.

 There's a comment in Equations.c that some of the equations were copied
 from
 Mathematica. Could that mathematica notebook be made available in the
 thorn
 as well?

 In the future it might be worth considering some way of merging this back
 into the original TwoPunctures, so we don't have multiple thorns
 implementing
 the same spectral solver infrastructure.

 There should be tests for all the different types of angular profiles for
 the
 initial data.

 The discussion in the README should be converted into a proper thorn
 documentation.


  Questions, comments and recommendations specifically to ProcaEvolve:

 Need a thorn documentation to relate variables to the paper and to explain
 the use of parameters.

 I don't know how much performance is a concern. If only a tiny amount of
 time
 is spent in Proca compared to BSSN (or other things) this is obviously not
 important. However, if the issues with the code, that prevents the
 compiler
 from optimizations, makes the code slow enough that the time spent in
 Proca is
 comparable to the time spent in BSSN it might be worth it to fundamentally
 rewrite the code. One of the problems, is the many nested loops over
 tensor
 indices. These prevents the compiler from vectorizing most of the RHS code
 (I checked this by looking at compiler vectorization output from the Intel
 compiler). As the loop lengths are just 3, the vectorizer deems that
 vectorization will be ineffecient and hence doesn't do it. On modern CPUs
 with
 vector lengths of 4 or 8 (in double precision), efficiently using the
 vector
 units is really important for good performance. However, this is not an
 issue
 (if there really is a problem with performance) that should prevent
 inclusion
 in the ET, but just something to think about for later.

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/2101#comment:4>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit