#2485: include complex and real scalar evolution code from Canuda in ET
Reporter: Roland Haas
Status: open
Milestone:
Version:
Type: task
Priority: major
Component:
Comment (by Giuseppe Ficarra):
Ok, so I think we are done, but let me know if you need me to do anything else.
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2485/include-complex-a…
#2485: include complex and real scalar evolution code from Canuda in ET
Reporter: Roland Haas
Status: open
Milestone:
Version:
Type: task
Priority: major
Component:
Comment (by Taishi Ikeda):
I do not know there is an available Mathematica code for it. But. then, we can forget my this comment. Now, I am thinking the calculations of eigen frequency is user’s responsibility….
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2485/include-complex-a…
#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-nrpyelliptice…
#2485: include complex and real scalar evolution code from Canuda in ET
Reporter: Roland Haas
Status: open
Milestone:
Version:
Type: task
Priority: major
Component:
Comment (by Giuseppe Ficarra):
Do you know how can I create such a list? Is there any Mathematica package that computes the eigenfrequencies? If this doesn’t take too much, I can try to make the list. Besides, the routine only implements the l = m = 1 mode, so this would only loop on the spin values.
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2485/include-complex-a…
#2549: inlcude FLRWSolver in ET
Reporter: Roland Haas
Status: new
Milestone: ET_2022_05
Version: development version
Type: enhancement
Priority: major
Component:
Comment (by Hayley Macpherson):
Chi - thanks for your review! I’ve addressed your comments, see below:
1\. Thanks for this, you’re right there was a minus sign missing which I’ve now fixed
2\. I’ve changed the name of this variable from kvalue → kdiag\_bg, since it represents the diagonal value of the extrinsic curvature K\_ij for the background \(not the trace\)
3\. Yes, you are correct that this choice of gauge means that for the background we have alpha → a and therefore coordinate time can be more or less interpreted as conformal time. I don’t think this choice of evolving lapse will give numerical errors \(which I don’t see, as far as I can tell\), because the actual time evolution is dependent on the time step in terms of coordinate time, dt, which is set via the grid spacing and the chosen Courant factor. This time step does not change as the lapse evolves, and I don’t think the lapse will have an impact here. I could be wrong, so @{557058:59e031ba-9bb5-4298-a472-7b99d0ae6f22} please correct me if so!
Thanks again for your time and for helping with this on such short notice!
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2549/inlcude-flrwsolve…
#2549: inlcude FLRWSolver in ET
Reporter: Roland Haas
Status: new
Milestone: ET_2022_05
Version: development version
Type: enhancement
Priority: major
Component:
Comment (by Hayley Macpherson):
@{557058:8bc23f2a-45c0-477d-8ac4-a5a16c734278} thanks for your review! I have addressed all of the issues and comments you have raised, and pushed the changes to the repo.
In addition to the changes to address your issues/comments, I have also re-named the source files and routines themselves to be more consistent with ET standards.
Here I’ll explain what I’ve done:
* Issue 1: removed these statements from param.ccl as they were unused anyway
* Issue 2: Roland said he would have a look into this, so I’ll leave this be for now
* Issue 3: added a test.ccl file
* Issue 4: removed doc/cactus.sty
* Issue 5: thanks for this! I’ve made a new routine FLRW\_ParamCheck and scheduled it at ParamCheck as you suggested, and removed this code from the ID routines
* Issue 6/Comment 2: I’ve added a comment on the example powerspectrum file in the documentation, and added a new section explaining each of the files in the tools/ directory.
* Issue 7: I removed doc/tutorial/ and moved the README.tutorial file to tools/
* Comment 3: I removed configs/ from the public repo and will consider contributing these to simfactory at some point
* Issue 8: re-named the test par to FLRW\_singlemode\_test.par since the main differences are the resolution and simplified initial parameters
* Issue 9: thanks for pointing this out. I’ve added a call to CCTK\_WARN after checking if the supplied file exists, with a useful error explaining next steps for the user
* Issue 10: removed the restart par file from par/
* Issue 11/Comment 4: thanks for this - I realised random.F90 was a lot more complicated than it needed to be. It already uses the intrinsic fortran PRNG, but had some added complexity in generating a seed if it wasn’t given. In FLRWSolver, there will always be a random seed. I removed random.F90 and replaced it with a new, much simpler routine in FLRW\_InitTools which directly calls the Fortran intrinsic PRNG and casts this into random numbers following a normal distribution
* Issue 12: Mescaline is my private post-processing code, so I’ve removed this comment
Thanks again for your help and effort on this! Let me know if there’s anything else or if you have questions about my changes.
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2549/inlcude-flrwsolve…
#2485: include complex and real scalar evolution code from Canuda in ET
Reporter: Roland Haas
Status: open
Milestone:
Version:
Type: task
Priority: major
Component:
Comment (by Taishi Ikeda):
> I agree with this comment, but in param.ccl there is a reference value for wR and wI for a specific case \(dimensionless spin 0.99 and mass parameter 0.42\). Moreover the paper on which the routine is based is cited in the README. My question is: do I need to add an explicit comment inside the source code about this?
I think it is not necessary to add comment in the source code. My point is preparing list of the eigenvalue with different spin parameter as separated text file….. But, the calculation of the eigen frequency may be just user’s responsibility. If you think so, please forget my this comment.
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2485/include-complex-a…
#2485: include complex and real scalar evolution code from Canuda in ET
Reporter: Roland Haas
Status: open
Milestone:
Version:
Type: task
Priority: major
Component:
Comment (by Giuseppe Ficarra):
> In ID\_SF\_BS, since wR and wI are free parameters, I recommend to add reference value as list for each spin parameter. Leaver method does not convergence for wR and wI which are not eigenvalue of the boundary value problem.
I agree with this comment, but in param.ccl there is a reference value for wR and wI for a specific case \(dimensionless spin 0.99 and mass parameter 0.42\). Moreover the paper on which the routine is based is cited more than once in the README. My question is: do I need to add an explicit comment inside the source code about this?
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2485/include-complex-a…
#2485: include complex and real scalar evolution code from Canuda in ET
Reporter: Roland Haas
Status: open
Milestone:
Version:
Type: task
Priority: major
Component:
Comment (by Taishi Ikeda):
> Do you mean “use\_jacobian = true“? If you have a look at line 254 of Scalar\_calc\_Tmunu.F90, you can see that there is an if statement that uses the combination "compute\_fluxes==1" and "use\_jacobian=false".
Oh, yes, sorry.. This is my typo. My point is that compute\_fluxes ==1 with use\_jacobian == true is not available. So, we should add comment saying “Flux can not be computed with multipatch“.
--
Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2485/include-complex-a…