#2616: Add NRPyEllipticET to the Einstein Toolkit

Reporter: Status: new Milestone: Version: Type: enhancement Priority: major Component:

Comment (by Cheng-Hsin Cheng):

Hi all, I finished checking NRPyEllipticET and I have some questions and comments about the code. To keep it brief I am leaving out the “conformally_flat_BBH” when referring to the file names in the src folder.

---

In NRPyEllipticET folder

* Please add a `README` in the Cactus format with the author, maintainer, license, and brief description.

* `interface.ccl`

* `uuGF`: Please add a description for it. Although it might be familiar for someone who has used TwoPunctures, a new user might not know what is the purpose of it.

* `param.ccl`:

* `info_output_freq`: I think a better description would be e.g. "Print progress of relaxation time evolution every info_output_freq iterations" * `N2`: allows a "forbidden value" of -1 to make sure it is set explicitly in the parfile, but the default is set to 16 rather than -1. Was the value of -1 to be removed (since N0 and N1 don't have a forbidden value like N2)? * `initialize_offdiagonal_metric_to_zero`: This looks to be unused in the code. Should it be removed from the param.ccl?

---

C source files

* `Hyberbolic_Relaxation.c`

* Using CCTK_ERROR (or CCTK_INFO) to error out might be preferable to using printf and calling exit(1). * Instead of hard-coding the value for integration_radius, could you add it as a thorn parameter?

* `conformally_flat_BBH_driver_bcstruct.c`

* Line 23: It took me a while to understand what the comment meant. I think it might be clearer to rewrite as "in the case that a boundary point is..." * Lines 59-61 are commented out. I don't know if `set_bcstruct` handles grids without the outer boundary, but if it doesn't, should this be left uncommented?

* `set_bcstruct.c`

* I really like how at the top there is a brief outline of the routine's (in addition to the comments within the body). It could be useful to add them for other longer routines, e.g. `Hyperbolic_Relaxation()` or `InitializeADMBase()`

* `find_timestep.c`

* The argument "CFL_FACTOR" isn't used in the function, moreover the variable is already defined in param.ccl, so is this required or can it be removed?

* `wavespeed_gf_all_points.c`

* There is duplicate code from `find_timestep.c` to compute ds_dirn0, ds_dir1, ds_dir2. Is it possible to implement this only once, e.g. write a separate function to compute min(ds_dirn0, ds_dir1, ds_dir2) for a single grid point, and then call it from both `find_timestep.c` and `wavespeed_gf_all_points.c`?

* `residual_all_points.c`

* The variables "f0_of_xx0" and so on are used in computing the residual, but they are defined with "__attribute__ ((unused))" in the headers in "rfm_files".

* `L2_norm_of_gf.c`

* What is the radius "r"? Could it be possible (and would it be a problem) for the extent of r to be under integration_radius, so that the integration is not performed over the whole the radius?

* Files in the folder `conformally_flat_BBH/rfm_files/`

* What is the purpose of the empty files with names ending in “read2”?

* Unused files: Should they be left out of the thorn?

* `apply_bcs_curvilinear_radiation.c` * `set_Cparameters-nopointer.h` * `set_Cparameters-SIMD.h` * `xx_to_Cart.c`

‌

-- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2616/add-nrpyellipticet...