Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add soliton potential for Bondi accretion problem #371

Merged
merged 105 commits into from
Jan 26, 2025

Conversation

sandy0216
Copy link
Contributor

No description provided.

Copy link
Contributor

@hsinhaoHHuang hsinhaoHHuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandy0216 Thanks for this pull request. It is a really interesting test problem.
I have reviewed it. Please let me know if my comments are unclear or if I got something wrong.

Besides the inline comments, I also have some comments here:

  1. Please also update the example/test_problem/Hydro/Bondi/README to describe the new soliton-potential model.
  2. I am not familiar with this test problem. I am wondering whether there is a way to know if the simulations run well, e.g., by comparing them to the analytical solution or analyzing the accretion rate. Or what should I expect to see the results of the new soliton-potential model?
  3. I am curious whether the Bondi_Soliton model is also a kind of hydrostatic equilibrium. If it is, why is it not implemented as a new Bondi_HSE_Mode? If it is not, what is its main difference to the three models of HSE_Mode?
  4. In SetGridIC(), there are 6 types of initial conditions: Bondi_HSE={1,2,3}, Bondi_Init=1, Bondi_Soliton=1, and the uniform. They are mutually exclusive, and there is a built-in order to decide which is adopted. For example, if the user sets Bondi_HSE=1; Bondi_Init=1; Bondi_Soliton=1;, it will only run Bondi_HSE=1, and the other two options will not actually be used. Users may not be aware of this. We have better tell the users about these rules in Input__TestProb and also check the input parameters and reset some of them to 0 in SetParameter() if there are conflicts. It would be better if we could make the dependency of parameters clear, like which parameter will be useless if which other parameter is off.
  5. For Bondi_Init mode, it requires an input table. We should provide a default one in example/test_problem/Hydro/Bondi/.
  6. The attached plotting scripts in example/test_problem/Hydro/Bondi/ seem outdated.
    • In plot_profile.gpt, we don't need ../../ in FILE_BONDI and DUMP_TABLE.
    • In plot_diagonal.gpt, the indices of some columns are wrong if there is the dual energy. The Mach number is 17 and the pressure is 14 now.
72c72
<         u (abs($4-CENTER)*3**0.5):16 w p pt 6 lc 6 tit 'Simulation' \
---
>         u (abs($4-CENTER)*3**0.5):17 w p pt 6 lc 6 tit 'Simulation' \
82c82
<         u (abs($4-CENTER)*3**0.5):13 w p pt 6 lc 6 tit 'Simulation' \
---
>         u (abs($4-CENTER)*3**0.5):14 w p pt 6 lc 6 tit 'Simulation' \
  1. GAMER has its own unique coding style. Although there is no official styling guide, we can try to follow a consistent code format. Therefore, please check again that the vertical alignment, the 3 spaces indentation, the comments, the variable names, and the spaces in if, for, etc. are consistent with elsewhere.
    • We don't want the trailing space in the .cpp file. We can check it by the command grep -r -n ' $' src/ in the terminal:
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp:79:                
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp:81:      } 
src/TestProblem/Hydro/Bondi/Flu_ResetByUser_Bondi.cpp:54:   if ( !Bondi_void ) 
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:412:         
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:433:      
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:599:      
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:627:   } 
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:764:   
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:781:   
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp:804:

src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Show resolved Hide resolved
Copy link
Contributor

@hyschive hyschive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Most comments raised by @hsinhaoHHuang have been adddressed by this follow-up PR.
  • Remaining tasks have been recorded here. @sandy0216 Please handle it when you have time.

src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/ExtAcc_Bondi.cpp Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
src/TestProblem/Hydro/Bondi/Init_TestProb_Hydro_Bondi.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@hyschive hyschive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandy0216 @hsinhaoHHuang Looks good. Thank you both for the excellent contribution and thorough review!

@hyschive hyschive merged commit b9d3fd3 into gamer-project:main Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement test Test problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants