[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 anonymous):

 I am reviewing the "LeanBSSNMoL" code for submission to the Einstein
 Toolkit. Executive summary: The code is beautifully written, but the
 thorn lacks test cases, which are necessary.



 Downloading and building the code worked fine.

 The code is written in Fortran 90. It has a clear structure and is
 very readable. It consists of essentially one large loop per function,
 which is efficient and compiles quickly, as opposed to using Fortran
 90's array syntax that unfortunately leads to bad slow-downs with some
 compilers.

 The code has the usual idiosyncracies where it expects that
 "CCTK_REAL" is the same as "double precision", and "CCTK_INT" is the
 same as "integer". These hold usually in practice, and the code is
 fine as is, although the main point of introducing "CCTK_REAL" and
 "CCTK_INT" was to allow these types to be distinct.



 I am reading the code, and I have the following remarks:

 The file "zero_rhs.F90" (and maybe others) do not include
 "cctk_Arguments.h". How do they even compile? The missing include
 statement should be added. I wonder whether there is a bug in the
 Einstein Toolkit in that including "cctk.h" already includes
 "cctk_Arguments.h"?

 I have, for obvious reasons, not thoroughly checked the
 implementations of finite differences and BSSN equations in the file
 "bssn_constraints.F90" and "calc_bssn_rhs.F90". I assume that the
 authors checked their code and am willing to trust them. Personally, I
 would have defined macros for the lengthy finite differencing
 expressions.

 I ran all test cases successfully. There are no test cases, so this is
 not a strong statement. Test cases need to be added for a few simple
 cases, e.g. for some of the Apples with Apples examples (nonlinear
 gauge wave, Gowdy spacetim) as well as a single, preferably rotating
 black hole.

 I have run the example parameter file on 1 node with 40 cores of the
 Niagara system of Compute Canada, using 10 MPI processes with 4 OpenMP
 threads each. This seems to work nicely, albeit progress was so slow
 that after a few hour only 16 iterations had completed, and horizons
 were found only once. A quick glance shows that the output looks
 correct.




 For future reference, example parameter files should contain comments
 near the top describing approximately how much memory and CPU time
 they use. This makes it much simpler to run them, and prevents people
 from accidentally running them on their laptop if there is no chance
 of success. Put differently, the driver should have a notion of the
 amount of memory available on a node (or laptop), and should refuse to
 allocate significantly more than that. Simfactory already has the
 necessary information.



 Additional remarks, as suggestions for future improvements to the
 authors:

 The file "evolve_utils.F90" uses "omp parallel do collapse(3)". It
 might be faster to use only "collapse(2)" here, and instead use "omp
 simd" for the innermost loop. The same applies to all OpenMP loops.

 Some functions have very generic names, such as "remove_trA" or
 "reset_detmetric". This risks name collisions throughout the
 executable. I suggest to add a prefix to these functions, even if only
 a short one such as "lbm_". Alternatively, these functions can be put
 into a module with a unique name such as "LeanBSSN_evolve_utils",
 which would have the added benefit of checking the function's
 signatures. (Scheduled functions cannot be put into modules.)



 Other, minor remarks:

 The thorn list entry contains the comment "Lean public thorns". This
 gives the impression as if there was a proper set of Lean thorns, and
 a few of them were made public. I'd prefer to call them simply "Lean
 thorns", as these thorns need to be interesting and useful by
 themselves, and should see some development by themselves. I object to
 the notion that Einstein Toolkit thorns are merely mirror images of
 thorns that truly live elsewhere, hidden, and which from time to time
 receive a data dump as upgrade. Instead, the thorns in the Einstein
 Toolkit should be the main development versions, and where private
 copies exist, they should contribute via pull request to the main
 development versions.

 The shift condition mentioned in the README of LeanBSSNMoL should
 recieve a proper pointer to a particular set of equations (e.g.
 (S2)?).

 I see comments such as "$Header:$" in some files. These are now-unused
 left-overs from CVS, and can be removed.

 Calls to "CCTK_WARN(0, ...)" can be replaced by "CCTK_ERROR(..."),
 which might be more readable.

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