(Sinktree) Wrong fallback behaviour when sink are not in cache#700
Conversation
…s in rkill option
There was a problem hiding this comment.
Summary of Changes
Hello @Yrisch, 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 addresses a critical bug in the sinktree method where the system was incorrectly handling cache misses for sink particle data, leading to the retrieval of information from the wrong memory arrays. My changes ensure that when sink particle data is not in the cache, the system correctly falls back to the dedicated sink particle data structures for accurate force computations. Additionally, I've refined the particle accretion logic to prevent unintended processing of sink particles in a general accretion routine.
Highlights
- Bug Fix: Sink Particle Data Retrieval: I've corrected the
compute_forcessubroutine insrc/main/force.F90. Previously, when data for a sink particle was not found in the cache, the system would incorrectly fall back to retrieving its coordinates (lines 1269-1277) and smoothing length (lines 1289-1299) from the generalxyzharray. This change ensures that if the neighbor is identified as a sink particle (iamsinkj), its data is correctly retrieved from thexyzmh_ptmassarray, preventing erroneous force calculations. - Sink Particle Accretion Logic Refinement: In
src/main/part.F90, I've removed a redundant or incorrect loop within theaccrete_particles_outside_spheresubroutine (lines 2111-2117). This loop was processingxyzmh_ptmass(which holds sink particle data) for accretion. The change ensures that this specific accretion routine now only applies to regular particles stored inxyzh, aligning with the expected behavior for sink particles which may have different accretion mechanisms.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request fixes an important bug in the sinktree method where a cache miss for a sink particle would lead to using an incorrect data array. The changes in force.F90 correctly address this by reordering the logic to check the cache first, and then fall back to the appropriate array for sink or SPH particles. This looks good.
I have one major point of feedback regarding a change in part.F90. A loop that deletes sink particles outside a certain radius has been removed. This is a significant functional change that is not mentioned in the pull request description. I've requested clarification to ensure this is intended and to suggest documenting it.
I also left a minor style comment in force.F90 to improve code consistency.
Description:
I discovered a wrong behaviour of the sinktree method while debugging #699. If we had a cache miss, the fallback was on the wrong array (if the neighbour was a sink). Difficult to detect that with tests, as the number of parts is set to 1000 (same as the cache size) in most cases.
Also removed the ptmass killing in 'accrete_particles_outside_sphere' as it had unexpected behaviour with the subgrouping module...
Components modified:
Type of change:
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? no
Is there a unit test that could be added for this feature/bug? yes
If so, please describe what a unit test might check:
We should increase the number of parts in testgrav