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