Collated merge of pull requests from the workshop#844
Merged
Conversation
Implemented the Reissner-Nordstrom metric and its derivatives in Cartesian-like and spherical coordinates - Define a dummy charge parameter in all metrices So the compiler won't complain for unused variables. - Integration tests for RN: CO in various charges.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a full Reissner–Nordström (RN) charged metric module ( ChangesReissner–Nordström Metric Implementation
moddump_addflyby Refactor and set_orbit API Expansion
Formatting, Housekeeping, and Tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a Reissner-Nordstrom (RN) metric module, adds trajectory caching in the binary black hole metric to improve performance, and updates the flyby moddump utility to support general orbital parameters. Critical feedback highlights that when adding a flyby, both the primary and secondary body positions and velocities must be updated to maintain the correct orbit. Additionally, using tiny(0.) as a tolerance for floating-point equality in setup checks is risky due to roundoff errors and should be replaced with a machine epsilon or a reasonable tolerance. Finally, intent(out) variables in the trajectory retrieval subroutine should be initialized to prevent undefined behavior if loading fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/growth.f90 (1)
199-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLabel the outward fragmentation threshold correctly.
The
elsebranch printsvfraginagain even though it reportsvfragoutSI/vfragout, so the diagnostic text is misleading.Fix
- write(iprint,"(2(a,1pg10.3),a)") ' vfragin = ',vfragoutSI,' m/s = ',vfragout,' (code units)' + write(iprint,"(2(a,1pg10.3),a)") ' vfragout = ',vfragoutSI,' m/s = ',vfragout,' (code units)'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/growth.f90` around lines 199 - 204, In the else branch of the if statement starting with isnow == 0, the second write statement has an incorrect label in its format string. It currently prints "vfragin" as the label, but the actual values being printed are vfragoutSI and vfragout. Change the label in the second write statement from "vfragin" to "vfragout" to correctly describe the outward fragmentation threshold values that are being displayed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/deriv.f90`:
- Line 143: The comment line at line 143 in src/main/deriv.f90 exceeds the
configured line-length limit for Fortitude, causing the linter to fail. Split
this single long comment into multiple shorter comment lines, each staying
within the line-length limit, while preserving the complete information about
the flag for pressure calculation from dens, the cons2primall call,
densityiterate function, and the eos_vars import details.
In `@src/main/growth.f90`:
- Around line 210-211: The write statements for the Vrel messages exceed the
Fortitude line width limit and the endif statements throughout the file are
missing proper spacing. Refactor the long write statements for Vrel (lines
checking ivrelkin == 0 and ivrelkin == 1) to fit within the width limit by using
line continuation or splitting the string literals across multiple lines, and
ensure all endif statements in the file have proper spacing before them as
required by Fortitude style guidelines. Apply these formatting fixes to the
locations at 210-211, 308-315, and 370-380.
In `@src/main/krome.F90`:
- Around line 142-147: In the dummy if statement block within the else
preprocessor directive (the block containing the comment about avoiding compiler
warning for unused variables), change the `endif` keyword to `end if` (using two
words instead of one). This applies to the line that terminates the if (.false.)
then block to comply with Fortran style requirements.
In `@src/main/metric_binarybh.f90`:
- Around line 123-138: The trajectory interpolation block in metric_binarybh
contains Fortitude linting violations that need to be corrected. First, replace
the single-quoted strings in the error call (the error function call with the
message about trajectory file) with double-quoted strings to match Fortran style
conventions. Second, the error message line is too long and exceeds the line
length limit, so split the error message construction across multiple lines
using continuation or break it into a separate string variable before passing it
to the error function. Finally, review the spacing around the endif at the start
of the block and the enddo at line 133 to ensure they follow consistent
formatting conventions with appropriate whitespace or comments as needed per
your project's Fortitude configuration.
- Around line 116-117: The update_metric function returns with ierr=0 (success)
even when trajectory loading or time validation fails, allowing binary-BH runs
to proceed with invalid data. In the load_trajectory section (around line
116-117), set ierr to a non-zero value when ntraj remains 0 after calling
load_trajectory, since this indicates the trajectory table is empty or invalid.
Similarly, for the out-of-range time clamping logic (lines 137-139 and
throughout lines 161-186), either set ierr to a non-zero value to signal failure
when time is out-of-range, or downgrade those error logs to warnings if
out-of-range clamping is intentional behavior. Ensure that any error condition
logged in this function results in a non-zero ierr value being returned to the
caller.
In `@src/main/metric_rn.f90`:
- Around line 7-22: The metric module needs to be brought into Fortitude
compliance by adding a default accessibility declaration (such as private)
following the module declaration at the top, ensuring the implicit none
statement is properly formatted with any required external attributes, fixing
the spacing and indentation of all endif statements throughout the file,
converting old-style array constructors to modern Fortran syntax in the affected
sections, replacing all single-quoted string literals with double quotes, and
refactoring line 331 to ensure it does not exceed 100 columns by breaking long
statements into multiple lines as appropriate.
In `@src/setup/setup_grtde.f90`:
- Around line 224-226: The charge variable is being unconditionally reset to 0.0
on line 225, which overwrites the value that was previously read from the setup
file. Remove the line that sets charge to 0.0 to preserve the charge value
loaded from the setup file. If initialization is needed for other cases, add a
conditional check to only reset charge when appropriate rather than
unconditionally discarding the setup file value.
In `@src/utils/moddump_addflyby.f90`:
- Around line 107-110: The prompt function call with orbit%input_type allows a
maximum value of 4 (the last parameter), but the prompt string only documents
options 0 through 3, creating a mismatch between what is shown to the user and
what is actually accepted. Either extend the prompt string to document what
option 4 represents (adding a new line with '4=<description>'), or reduce the
maximum allowed value from 4 to 3 in the prompt function call to ensure the
interactive input options match the documentation.
- Around line 80-88: The issue is that body 2 is being copied from set_orbit
output without validating the orbit state and without checking for errors. After
the set_orbit call in the block with parameters including nptmass_in,
xyzmh_ptmass_in, vxyz_ptmass_in, and ierr, check the ierr status before
proceeding with the copy operations. Additionally, initialize the scratch
buffers xyzmh_ptmass_in and vxyz_ptmass_in before the set_orbit call to ensure
clean state. Finally, verify or adjust the primary sink (first body) position
and velocity to match what set_orbit generated for body 1, so that when body 2
is copied to nptmass, it represents the correct relative orbit with respect to
the actual primary that will be used.
In `@src/utils/phantom_moddump.f90`:
- Line 26: The `use io` import statement with all its imported entities
(set_io_unit_numbers, iprint, idisk1, warning, fatal, iwritein, id, master,
fileprefix) exceeds the 100-character line limit at 104 characters. Split this
use statement across multiple lines by wrapping the list of imported entities,
following Fortran's standard syntax for continuation lines, to ensure no single
line exceeds the 100-character limit while maintaining the same imports.
---
Outside diff comments:
In `@src/main/growth.f90`:
- Around line 199-204: In the else branch of the if statement starting with
isnow == 0, the second write statement has an incorrect label in its format
string. It currently prints "vfragin" as the label, but the actual values being
printed are vfragoutSI and vfragout. Change the label in the second write
statement from "vfragin" to "vfragout" to correctly describe the outward
fragmentation threshold values that are being displayed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86e1360b-3d4e-46a2-9761-dd974d4b2827
📒 Files selected for processing (45)
AUTHORSbuild/Makefilescripts/no-dims.plscripts/writemake.shsrc/main/H2regions.f90src/main/checksetup.f90src/main/dens.F90src/main/deriv.f90src/main/eos.f90src/main/eos_HIIR.f90src/main/eos_helmholtz.f90src/main/extern_gr.f90src/main/externalforces.f90src/main/externalforces_gr.f90src/main/force.F90src/main/growth.f90src/main/krome.F90src/main/metric_binarybh.f90src/main/metric_et.f90src/main/metric_kerr-schild.f90src/main/metric_kerr.f90src/main/metric_minkowski.f90src/main/metric_rn.f90src/main/metric_schwarzschild.f90src/main/metric_tools.f90src/main/porosity.f90src/main/readwrite_dumps.f90src/main/substepping.F90src/setup/relax_star.f90src/setup/set_orbit.f90src/setup/setup_dustgaussvel.f90src/setup/setup_dustybox.f90src/setup/setup_dustysedov.f90src/setup/setup_grtde.f90src/setup/setup_testgrowthsphere.f90src/tests/test_eos.f90src/tests/test_gr.f90src/utils/analysis_growingdisc.f90src/utils/analysis_maxquant.f90src/utils/moddump_addflyby.f90src/utils/moddump_centreonsink.f90src/utils/moddump_removeparticles_density.f90src/utils/moddump_rotate_ref.f90src/utils/phantom_moddump.f90src/utils/utils_disc.f90
Comment on lines
+7
to
+22
| module metric | ||
| ! | ||
| ! None | ||
| ! | ||
| ! :References: None | ||
| ! | ||
| ! :Owner: Angelos Karakonstantakis | ||
| ! | ||
| ! :Runtime parameters: | ||
| ! - charge : *charge parameter for Reissner-Nordstrom metric* | ||
| ! - mass1 : *black hole mass in code units* | ||
| ! | ||
| ! :Dependencies: dump_utils, infile_utils, io | ||
| ! | ||
| implicit none | ||
| character(len=*), parameter :: metric_type = 'RN' |
There was a problem hiding this comment.
Bring the new RN module into Fortitude compliance.
Fortitude reports errors in this new file: missing default accessibility, implicit none missing external, endif spacing, old-style array constructors, single-quoted literals, and Line 331 exceeding 100 columns. Apply the configured style or explicitly suppress metric modules if they are intentionally exempt.
Also applies to: 97-150, 331-331, 364-390, 406-422, 437-439
🧰 Tools
🪛 Fortitude (0.9.0)
[error] 7-8: module 'metric' missing default accessibility statement
(C131)
[error] 21-21: 'implicit none' missing 'external'
(C003)
[error] 22-22: String uses single quotes but double quotes preferred
(S241)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/metric_rn.f90` around lines 7 - 22, The metric module needs to be
brought into Fortitude compliance by adding a default accessibility declaration
(such as private) following the module declaration at the top, ensuring the
implicit none statement is properly formatted with any required external
attributes, fixing the spacing and indentation of all endif statements
throughout the file, converting old-style array constructors to modern Fortran
syntax in the affected sections, replacing all single-quoted string literals
with double quotes, and refactoring line 331 to ensure it does not exceed 100
columns by breaking long statements into multiple lines as appropriate.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
This is a merge to resolve build failures and merge two pull-requests leftover from the workshop #832 and #839. Also contains a run of the bots, and a minor update to metric_binarybh to read the trajectory file into a memory buffer instead of continuously rewriting the file.
Also contains a post-workshop run of the bots, including a bug fix to the align-bot
Components modified:
Type of change:
Testing:
Should pass continuous integration tests
Did you run the bots? yes
Did you update relevant documentation in the docs directory? no
Did you add comments such that the purpose of the code is understandable? yes/no
Is there a unit test that could be added for this feature/bug? yes/no
If so, please describe what a unit test might check:
n/a
Related issues: #835
#839
#826
Summary by CodeRabbit
Release Notes
chargein metric and setup inputs..moddumpfile.