include complex and real scalar evolution code from Canuda in ET
No description provided.
Comments (32)
-
-
reporter - changed status to open
-
reporter Has this already been included? If not should it be added as an inclusion candidate for the next ET release?
-
As far as I see, everything is ready for inclusion.
-
reporter Ok, I have added it to the list of suggested inclusions for the next ER release https://docs.einsteintoolkit.org/et-docs/Meeting_agenda#2022-06-23 if you can, please feel free to joing the ET call next week (Thu June 23rd 8:30am CDT).
-
reporter -
assigned issue to
-
assigned issue to
-
reporter Taishi Ikeda agreed to review the code, Miguel volunteered to be champion.
-
reporter Draft review guidelines are at https://docs.einsteintoolkit.org/et-docs/How_to_Review_a_new_component
-
reporter If possible please include both senior authors (Miguel, Helvi) in all communications.
-
Hi, I checked the thorn, and I have several comments. Now, there is not test case, my comment is only about code now. Since I am checking development branch, the line and file name are based on development branch.
In ScalarBase,
-
In line 16th in param.ccl in ScalarBase, we should add absolute value for phi.
-
The description of parameters lEF and mEF in param.ccl is misleading. Current version supports only (lEF,mEF)=(0,0), (1,1), (2,2) and not (1,0),(2,0), (2,1). (see 68th line in Scalar_rhs_forcing.F90)
-
Is there information about external force as document ? I can not find it.
In ScalarEvolve,
-
In ScalarEvolve, why BSSN-like variables are public ? Since same name variables are used in other thorn (like lean_public) and they are just auxiliary field to calculate rhs of the equations, private is better for them.
-
In Scalar_calc_Tmunu.F90, in Scalar_calc_Tmunu, xx and rr should be private in OpenMP in grid loop.
-
In current version, in Scalar_calc_Tmunu, "compute_fluxes==1" and "use_jacobian=false" is not available....Should we add the comments in param file ?
-
In Scalar_ord4_calc_rhs.F90, line 638th and 639th (def sn1 and sn2) can be outside grid loop ? or should the excision procedure be separated as independent thorn ?
-
Also, Rout_excision1, Rout_excision2, Rin_excision1, Rin_excision2 can be outside grid loop ?
-
In schedule, flux grid function is allocated, and Scalar_zero_densities is called when compute_fluxes is true. But, in Scalar_zero_densities, flux grid functions are initialized only when use_jacobian is false. So, if compute_fluxes is true and use_jacobian is true, flux grid function is not initialized.
In ScalarInit
-
In ID_SF_BS, bh mass assumed to be one ? There is parameter m_plus, but, it is not used appropriately.... for example, line 40th and 41th.
-
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.
-
In schedule, comment in line 51th is wrong.
-
The description of parameter ampSF is "amplitude of Gaussian wave packet". The parameter is used in "ID_SF_calc_Gaussian.c", but also "ID_SF_calc_Const.c". In second case, the description for the parameter is wrong.
-
-
In ScalarEvolve, why BSSN-like variables are public ? Since same name variables are used in other thorn (like lean_public) and they are just auxiliary field to calculate rhs of the equations, private is better for them.
fixed in https://bitbucket.org/canuda/scalar/commits/98828a12f314e91e3af135a4f4b58743d47a32fc
In Scalar_calc_Tmunu.F90, in Scalar_calc_Tmunu, xx and rr should be private in OpenMP in grid loop.
fixed in https://bitbucket.org/canuda/scalar/commits/44027452fc248b00e2e99b31d42cb23215c2aad1
-
ScalarBase:
In line 16th in param.ccl in ScalarBase, we should add absolute value for phi.
fixed by Miguel in https://bitbucket.org/canuda/scalar/commits/345ea062f4b055a50dfeec74308899f9d97fd788
The description of parameters lEF and mEF in param.ccl is misleading. Current version supports only (lEF,mEF)=(0,0), (1,1), (2,2) and not (1,0),(2,0), (2,1). (see 68th line in Scalar_rhs_forcing.F90)
fixed in https://bitbucket.org/canuda/scalar/commits/2edee1a8386be2d7bee58c457c330cee843a6b7b
Is there information about external force as document ? I can not find it.
added in https://bitbucket.org/canuda/scalar/commits/e56406521d1d842910987d430f2c1695cf84ab00. This is a very short description, I am not sure if I need to be more specific and add equations or descriptions of the choices available, please let me know what you think.
ScalarInit:
In ID_SF_BS, bh mass assumed to be one ? There is parameter m_plus, but, it is not used appropriately.... for example, line 40th and 41th.
yes, m_plus is assumed to be = 1, ID_SF_BS is used to compute the quasi-bound states of a massive scalar field around a single rotating BH following Sam Dolan’s paper: https://arxiv.org/pdf/0705.2880.pdf. For the implementation details, I would ask Helvi since she’s the one who did it.
In schedule, comment in line 51th is wrong.
fixed in https://bitbucket.org/canuda/scalar/commits/9032bbf9d7318083e337ad45cb55534e53653b85
The description of parameter ampSF is "amplitude of Gaussian wave packet". The parameter is used in "ID_SF_calc_Gaussian.c", but also "ID_SF_calc_Const.c". In second case, the description for the parameter is wrong.
fixed in https://bitbucket.org/canuda/scalar/commits/b117a37827f1555b20de2e7bf2b7ce5d661c1da5
-
reporter Please provide the “does no harm” initial review verdict before the deadline of Sept. 1st 2022.
-
reporter Taishi confirmed (private email) that the thorns do no harm.
-
Also, Rout_excision1, Rout_excision2, Rin_excision1, Rin_excision2 can be outside grid loop ?
In Scalar_ord4_calc_rhs.F90, line 638th and 639th (def sn1 and sn2) can be outside grid loop ? or should the excision procedure be separated as independent thorn ?
fixed in https://bitbucket.org/canuda/scalar/commits/f0e3b8f2d7c7d943f8002d1f72944046fabe29ef
-
added test parameter files in https://bitbucket.org/canuda/scalar/commits/04708a96e8c10f0b3dcefc17e59fcf9ad7e78c33
-
ScalarEvolve:
In current version, in Scalar_calc_Tmunu, "compute_fluxes==1" and "use_jacobian=false" is not available....Should we add the comments in param file ?
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".
in schedule, flux grid function is allocated, and Scalar_zero_densities is called when compute_fluxes is true. But, in Scalar_zero_densities, flux grid functions are initialized only when use_jacobian is false. So, if compute_fluxes is true and use_jacobian is true, flux grid function is not initialized.
fixed in https://bitbucket.org/canuda/scalar/commits/cb03ec3b4d1bd2e32787ffbf2d32f4dae2e13f27
-
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“.
-
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“.
perfect, thanks! comment added in https://bitbucket.org/canuda/scalar/commits/aa56f4304016d6366969c7cd4cd4d95c88eeca8f
-
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 in the README. My question is: do I need to add an explicit comment inside the source code about this?
-
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.
-
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 and mass parameter values.
-
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….
-
Ok, so I think we are done, but let me know if you need me to do anything else.
-
In ID_SF_BS, bh mass assumed to be one ? There is parameter m_plus, but, it is not used appropriately.... for example, line 40th and 41th.
fixed in https://bitbucket.org/canuda/scalar/commits/6e3ea2ea08b88d4d4b2f2cd2a9d0e42e34a9e011
-
reporter @Taishi Ikeda could you confirm for the record that the review concluded?
-
@Taishi Ikeda could you confirm for the record that the review concluded?
I finished my review process for the thorn
-
reporter I just notice that none of the Scalar* thorns have a documentation.tex file. Those files are required for inclusion and them not being there should have precluded acceptance (and their lack was mentioned in the vote but then not acted on, sorry).
The release date has been pushed by a week, so if you can please do provide some documentation files as soon as possible.
-
reporter @Miguel Zilhão before the next release (but as soon as possible) if you could take a stab a updating the basic docs that I had cobbled together as well as (and that is actually quite important) add a test case for the arrangement (one is enough, pick any thorn to contain it as long as it tests all 3 thorns), that would be great.
-
Sure, I’ll update the docs right after the Christmas break. Concerning the test cases, these have been added already. @Giuseppe_Ficarra added them here: https://bitbucket.org/canuda/scalar/src/master/ScalarEvolve/test/
Are these not enough?
-
reporter Yes, those are enough. Not sure why I thought they were missing (given that they have commit dates 4 months ago). So only a closer look at documentation would be needed.
-
reporter @Miguel Zilhão is there any chance of seeing improve documentation? Right now it is very bare bones since it was just copy and pasted from the paper by a non-author (myself). Eg right now there is docs for the Scalar arrangement in:
http://einsteintoolkit.org/arrangementguide/Scalar/documentation.html
and then TwoPunctures in http://einsteintoolkit.org/thornguide/Scalar/TwoPunctures_BBHSF/documentation.html but nothing for ScalarEvolve etc. This makes the docs page look decidedly odd:
http://einsteintoolkit.org/documentation/ThornGuide.php
- Log in to comment
link to the code repository: https://bitbucket.org/canuda/scalar/src/master/