1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

include complex and real scalar evolution code from Canuda in ET

Issue #2485 open
Roland Haas created an issue

No description provided.

Comments (32)

  3. Roland Haas reporter

    Has this already been included? If not should it be added as an inclusion candidate for the next ET release?

  10. Taishi Ikeda

    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.

    ‌

  11. Miguel Zilhão

    ‌ 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

  12. Giuseppe Ficarra

    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

  13. Roland Haas reporter

    Please provide the “does no harm” initial review verdict before the deadline of Sept. 1st 2022.

  17. Giuseppe Ficarra

    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

  18. 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“.

  20. 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 in the README. My question is: do I need to add an explicit comment inside the source code about this?

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

  22. 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 and mass parameter values.

  23. 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….

  27. Taishi Ikeda

    @Taishi Ikeda could you confirm for the record that the review concluded?

    I finished my review process for the thorn

  28. Roland Haas 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.

  29. Roland Haas 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.

  31. Roland Haas 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.

  32. Roland Haas 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

    ‌

  33. Log in to comment
Assignee
Type
task
Priority
major
Status
open
Component
–
Milestone
–
Version
–
Votes
0
Watchers
4
Jira Software: the preferred issue tracker for Bitbucket. Join the team!