#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.