Major cleanup of initial and evolve modules#718
Conversation
…ly the dump write
… and group ptmass calls together
…solete/unused ptmass_not_obscured function
…pdate_dump_counters routines; added reset_timers routine to simplify code
…ubroutines; no change in functionality
There was a problem hiding this comment.
Summary of Changes
Hello @danieljprice, 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 focuses on enhancing the modularity and maintainability of the codebase through a substantial refactoring effort, particularly within the core initial and evolve modules. By decomposing complex routines into more focused subroutines, the code becomes easier to understand and manage. Alongside this structural improvement, a critical bug affecting velocity derivative calculations under specific conditions has been resolved, and new utilities for analyzing the spatial distribution of particles have been introduced. The changes aim to improve the overall robustness and clarity of the simulation framework.
Highlights
- Major Refactoring: The
initial.F90andevolve.F90modules have undergone significant refactoring, breaking down large, monolithic subroutines into smaller, more modular and manageable routines. This improves code organization and maintainability without altering functionality. - Bug Fix: A bug in
dens.F90was fixed wherepmassiwas incorrectly used instead ofpmassjin the calculation of velocity derivatives when using the APR (Adaptive Particle Refinement) scheme. - Particle Extent Calculation: New functionality has been added to
centreofmass.f90to compute and print the spatial extent (min/max coordinates and dimensions) of the particle distribution, excluding dead or accreted particles. - Streamlined Conservation Checks and Dump Writing: The logic for conservation checks and dump file writing has been streamlined by consolidating related operations into new, dedicated wrapper subroutines, improving clarity and reducing redundancy.
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. 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
-
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 is a significant and well-executed refactoring of the initial.F90 and evolve.F90 modules. By breaking down these large modules into smaller, more manageable subroutines, you've greatly improved the code's readability, modularity, and maintainability. This is a fantastic cleanup effort.
The pull request also includes a critical bug fix in src/main/dens.F90, correcting the use of pmassi to pmassj in the velocity derivative calculation, which is an important correction.
My review focuses on ensuring the refactoring is clean and doesn't introduce new issues. I have one minor suggestion in src/main/initial.F90 to rename a subroutine to avoid potential confusion with another one of the same name, further enhancing clarity.
…e declared at top of module
…; shifted utility routines to separate module; delete obsolete libphantom-evolve.F90
Description:
This is a refactoring effort to break up two of the major vomit-on-the-page modules (initial.F90 and evolve.F90) into manageable, smaller and modular chunks. There is no change in functionality.
The p-r also fixes a small bug in computing velocity derivatives when APR is employed, where there was a pmassi instead of a pmassj in the density routine
Components modified:
Type of change:
Testing:
Code compiles and appears to work as previously
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:
Related issues: #55