[ET Trac] #2616: Add NRPyEllipticET to the Einstein Toolkit

[Cheng-Hsin Cheng]

#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-to-the-einstein-toolkit
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.einsteintoolkit.org/pipermail/trac/attachments/20220907/7691f5ac/attachment.html