Skip to content

Implementation of Reissner-Nordstrom metric in gr.#832

Open
karakonang wants to merge 1 commit into
danieljprice:mainfrom
karakonang:rebase-v2026.0.1
Open

Implementation of Reissner-Nordstrom metric in gr.#832
karakonang wants to merge 1 commit into
danieljprice:mainfrom
karakonang:rebase-v2026.0.1

Conversation

@karakonang

Copy link
Copy Markdown

Description:

Implemented the Reissner-Nordstrom metric and its derivatives in Cartesian-like and spherical coordinates.
metric charge parameter in code units can be interpreted as charge to mass ration (when mass1=1 in code units).
TODO: documentation

Components modified:

  • Setup (src/setup)
  • Main code (src/main)
  • Test suite (src/tests)

Type of change:

Testing:

  • GR test for precession for zero charge => equivalent to Schwarzschild.
  • Custom test over various charges (partially added --need to be checked--.

Did you run the bots? yes/no

Did you update relevant documentation in the docs directory? yes/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:

Related issues: #

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 the Reissner-Nordstrom (RN) metric to the codebase, adding a new metric_rn.f90 module, integrating a charge parameter across other metric and setup files, and introducing circular orbit tests for the RN metric. Feedback on these changes highlights a critical bug where imetric is misconfigured as 4 instead of 8 in metric_rn.f90, which will cause the metric to be incorrectly identified as Kerr-Schild. Additionally, the reviewer recommends removing several unused variables in metric_cartesian_derivatives, replacing hardcoded numerical values of pi in the test suite with the imported pi constant, and replacing a Unicode en-dash with a standard hyphen in a string literal to prevent compiler compatibility issues.

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.

Comment thread src/main/metric_rn.f90 Outdated
Comment thread src/main/metric_rn.f90
Comment on lines +168 to +187
real :: x,y,z,r,r2,r3,r4,r5,rs_on_r3,x2,y2,z2,rs2
real :: rs
rs = 2.*mass1

dgcovdx = 0.
dgcovdy = 0.
dgcovdz = 0.
x = position(1)
y = position(2)
z = position(3)
x2= x**2
y2= y**2
z2= z**2
r2 = dot_product(position,position)
r = sqrt(r2)
r3 = r*r2
r4 = r2*r2
r5 = r*r4

rs_on_r3 = rs/r3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The variables r5, rs_on_r3, and rs2 are declared and some are calculated (lines 185, 187) but they are never used in metric_cartesian_derivatives. Removing these unused variables and their calculations improves code maintainability and readability.

 real :: x,y,z,r,r2,r3,r4,x2,y2,z2
 real :: rs
 rs = 2.*mass1

 dgcovdx = 0.
 dgcovdy = 0.
 dgcovdz = 0.
 x = position(1)
 y = position(2)
 z = position(3)
 x2= x**2
 y2= y**2
 z2= z**2
 r2 = dot_product(position,position)
 r  = sqrt(r2)
 r3 = r*r2
 r4 = r2*r2

Comment thread src/tests/test_gr.f90
Comment on lines +615 to +617
use io, only:id,master
use metric_tools, only:imetric,imet_rn
use metric, only:mass1,charge

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Import the named constant pi from the physcon module to avoid hardcoding the numerical value of $\pi$ in the calculation of the orbital period.

 use io,           only:id,master
 use physcon,      only:pi
 use metric_tools, only:imetric,imet_rn
 use metric,       only:mass1,charge

Comment thread src/tests/test_gr.f90
! v_circ = sqrt( M/r - Q^2/r^2 )
v0 = sqrt( mass_val/r0 - charge_val**2/r0**2 )

period = 2.0 * 3.14159265358979 * r0 / v0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Use the named constant pi imported from physcon instead of the hardcoded numerical value 3.14159265358979 to ensure consistency and precision.

    period = 2.0 * pi * r0 / v0

Comment thread src/main/metric_rn.f90 Outdated
write(iunit,"(/,a)") '# options relating to the '//trim(metric_type)//' metric'

call write_inopt(mass1,'mass1','black hole mass in code units',iunit)
call write_inopt(charge,'charge','charge parameter for Reissner–Nordstrom metric',iunit)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a standard hyphen - instead of the Unicode en-dash (U+2013) in the string literal is safer for compatibility with older compilers or environments that might not fully support UTF-8 in source files.

 call write_inopt(charge,'charge','charge parameter for Reissner-Nordstrom metric',iunit)

@karakonang karakonang marked this pull request as ready for review June 11, 2026 06:17
@danieljprice danieljprice changed the title Implematation of Reissner-Nordstrom metric in gr. Implementation of Reissner-Nordstrom metric in gr. Jun 11, 2026
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.
@danieljprice

Copy link
Copy Markdown
Owner

@karakonang remaining failure is in metric_binarybh.f90; needs charge defined at the top of the routine. Should be minor fix...

../src/main/externalforces_gr.f90:22:26:

 22 |  use metric, only:mass1,a,charge
    |                          1

Error: Symbol ‘charge’ referenced at (1) not found in module ‘metric’

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants