-
Notifications
You must be signed in to change notification settings - Fork 321
winds from various sinks #746
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
base: master
Are you sure you want to change the base?
Conversation
…n --> use_icosahedron, npart_inj -> ieject
Summary of ChangesHello @lsiess, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the simulation's capabilities in modeling stellar winds by enabling multiple wind-emitting sink particles, each with customizable wind and spin characteristics. It introduces advanced particle injection techniques, such as Fibonacci spheres and specialized methods for jets and rotating winds, to create more accurate initial conditions. Concurrently, the underlying physics has been improved with new high-temperature cooling and heating mechanisms, ensuring a more comprehensive treatment of thermal processes in these dynamic environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant enhancements and refactoring to support winds from multiple sinks, alongside new physics for high-temperature cooling and more flexible particle injection methods. The overall direction is excellent, improving modularity and physical realism. However, I've identified a few critical issues where the implementation for multiple sinks is not fully generalized and contains hardcoded limitations that could lead to incorrect behavior with more than two sinks. There are also some minor bugs and areas for improvement in the new cooling functions. Addressing these points will greatly improve the robustness and scalability of these new features.
src/main/inject_wind.f90
Outdated
| integer, parameter :: nrho_index = 10 | ||
| integer :: k | ||
| real :: inner_radius,r3 | ||
| integer :: k,isink=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sink index isink is hardcoded to 1 within the pulsating_wind_profile subroutine. This will cause incorrect behavior when a pulsating wind is used with any sink other than the first one, as it will use the physical parameters of sink 1.
To fix this, isink should be passed as an argument to this subroutine. The call site in inject_particles is within a loop over sinks, so the isink loop variable is available to be passed.
integer :: k,isink
| real :: Mstar_cgs, Lstar_cgs, wind_gamma, Mdot_cgs, Tstar, Rstar | ||
| real :: u_to_temperature_ratio | ||
| real, dimension (:,:), allocatable, public :: trvurho_1D, JKmuS_1D | ||
| real, dimension (:,:), allocatable, target, public :: trvurho_1D,trvurho_1D2,JKmuS_1D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation for storing 1D wind profiles is limited to a maximum of two sinks by using trvurho_1D and trvurho_1D2. This approach is not scalable for more than two wind-emitting sinks.
To properly support winds from various sinks, this should be generalized. A better approach would be to use a dynamically allocated array of pointers, where each element can point to the wind profile for a specific sink. For example:
type(wind_profile_t), pointer :: trvurho(:) => null()
...
allocate(trvurho(nptmass))This would make the implementation more robust and scalable.
| ! could compute analytical formula but not needed for implicit cooling | ||
| dlnQ_dlnT = 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting dlnQ_dlnT = 0. is acceptable for the implicit cooling solver, as the comment indicates. However, this will lead to inaccuracies if the exact cooling solver (icool_method = 2) is used, as it relies on this derivative. To ensure correctness across all solver options, it would be better to compute the analytical derivative of Q_cgs with respect to T or use a numerical approximation.
src/main/wind.F90
Outdated
| case (3) | ||
| state%alpha = state%alpha_Edd+alpha_rad | ||
| state%dalpha_dr = (state%alpha_Edd+alpha_rad-alpha_old)/(1.e-10+state%r-state%r_old) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ss sink particle arrays inside utility routines; fix issues with bondi injection
…rom desired dr/dp and specified particle mass
Description:
Components modified:
Type of change:
Testing:
Did you run the bots? no
Did you update relevant documentation in the docs directory? no
Did you add comments such that the purpose of the code is understandable? yes
Is there a unit test that could be added for this feature/bug? yes
If so, please describe what a unit test might check:
a test should check wind values (rho, velocity) against reference values when running phantom on SETUP=wind with more than one wind emitting sink
Related issues: #