[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 miguel.zilhao.nogueira):

 many thanks for the comments and review! below we try to address your
 points.

 > 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"?

 indeed this was missing; it has been added.

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

 using macros would have been preferred here, yes. the reason for us not
 doing so
 is partly historical, as the code we built upon used the present
 implementation
 (and had already extensively been tested). so we decided to keep this.

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

 we had only one test in place, which is triggered through the ProcaEvolve
 thorn. we have now added "standalone" tests, as well as one test for the
 NPScalars thorn.

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

 we've added the information with the memory consumption for our sample
 parameter
 file just before the grid specification.

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

 we have now added the simd directives to the loops, thanks.

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

 since these functions are scheduled, we have opted to add the prefix
 "LeanBSSN_".

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

 we have removed this thornlist, actually, since it contained the rest of
 the
 (out-of-date) ET thorns. we have added the comment "Lean thorns" to the
 thornlist with just the Lean thorns.

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

 this has been added.

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

 these have been removed.

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

 we have done this replacement throughout.

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