Zero Temperature eos#842
Conversation
…ion from kippenhahn in it. I just need to derive sound speed and add it there along with internal energy and a reverse function to find density from pressure, along with options in eos.f90
…or density from pressure. also decided to add chandra eos function to make things easier
…places possibly needed
…y testing it if there is anything else he thinks is necessary
…, with the default set to c=0.5 0=0.5
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new zero-temperature equation of state (EOS type 25) for white dwarfs, implemented in eos_zerotemp.f90 and integrated into the main eos.f90 module and build configuration. The review feedback highlights several critical areas for improvement, primarily focusing on adding defensive guards against zero or negative density and pressure values to prevent division-by-zero and mathematically undefined operations. Additionally, the feedback suggests using Taylor series expansions to prevent catastrophic cancellation at low densities in f_chandra and get_zerotemp_u, analytically simplifying the sound speed calculation, and immediately aborting with a fatal error if the mass fractions do not sum to one.
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.
…expression for example
|
@AliPourmand this looks pretty good but would it be possible to add some unit tests of this Eos in test_eos.f90? For example, a simple one would be to check that calling eos_helmholtz with temperature of zero gives the same answer... |
|
I don't know if Coulomb interactions would still play a role with Helmholtz EOS at T=0 though, so my guess is that isn't a good test. ( actually when i relax the same MESA star with both helmholtz and zerotemp, helmholtz recreates the MESA star accurately at the core where it is T~0 but zero temp doesn't and part of the reason is probably what I said) |
… test in test_setstar. the subroutine there called test_whitedwarf will relax the star with eos helmholtz and eos zerotemp. not done testing it yet though
…ste stuff into the root directory to test, but this issue I had before too so its fine
|
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:
📝 WalkthroughWalkthroughA new Fortran module ChangesZero-temperature EOS (ieos=25)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/eos_zerotemp.f90`:
- Around line 105-110: The species mass fraction variables (xh, xhe, xc, xo,
xne, xmg) are read using read_inopt without any validation of their bounds,
allowing invalid inputs like negative values to pass through. After each
read_inopt call for these variables, add validation checks to ensure each mass
fraction is within a valid physical range (typically 0.0 to 1.0). If a value is
out of range, increment nerr and log an appropriate error message to alert the
user to the invalid input before further calculations that depend on these
fractions are performed.
- Around line 267-295: When presi is zero or negative, the density inversion
should return zero density without attempting the bisection loop, as this
boundary case causes xmid to approach zero and the convergence criterion at line
290 fails due to division by near-zero. Add an explicit check for presi <= 0
before the bisection loop (before setting up xlo and xhi), and if presi is
non-positive, set the result to zero density (xmid = 0.0) and skip the iterative
algorithm entirely, avoiding the problematic division in the convergence check.
In `@src/tests/test_setstar.f90`:
- Line 478: The call to update_test_scores function at line 478 is a duplicate
that appears without any new assertion between it and a previous invocation,
causing test results to be double-counted. Remove this redundant call to
update_test_scores to ensure each test result is counted only once.
- Around line 441-458: The variable expected_error is used uninitialized in the
checkval call within the set_star test block, making the assertion
nondeterministic. Initialize expected_error to 0 before calling init_eos.
Additionally, add a radiation-conflict check after init_eos similar to the
pattern in test_redsupergiant that short-circuits and exits the current
iteration when radiation is enabled with an unsupported EOS, preventing the
set_star call and subsequent assertions from executing in those cases.
🪄 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: 2f62b624-a827-4263-86f9-6e4ef7d0b0e5
📒 Files selected for processing (5)
build/Makefilesrc/main/datafiles.f90src/main/eos.f90src/main/eos_zerotemp.f90src/tests/test_setstar.f90
| if (id==master) write(*,"(/,a,i2/)") '--> Testing ieos = ',ieos | ||
| call init_eos(ieos,ierr) | ||
|
|
||
| rmserr = 0. | ||
| call set_star(id,master,star,xyzh,vxyzu,eos_vars,rad,& | ||
| npart,npartoftype,massoftype,hfact,& | ||
| xyzmh_ptmass,vxyz_ptmass,nptmass,ieos,gamma,X_in,Z_in,& | ||
| relax=relax_star,use_var_comp=var_comp,write_rho_to_file=.false.,& | ||
| rhozero=rhozero,npart_total=ntot,mask=i_belong,ierr=ierr,& | ||
| write_files=.false.,density_error=rmserr,energy_error=ekin) | ||
|
|
||
|
|
||
| call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr') | ||
| call update_test_scores(ntests,nfail,npass) | ||
|
|
||
| call check_setup(nerror,nwarn,restart=.false.) | ||
| call checkval(nerror+nwarn,0,0,nfail(1),'no errors or warnings') | ||
| call update_test_scores(ntests,nfail,npass) |
There was a problem hiding this comment.
Initialize expected_error before assertion and mirror radiation-conflict handling.
At Line 453, expected_error is used uninitialized, so this check is nondeterministic. Also, when radiation is enabled with unsupported EOS, this test should short-circuit like test_redsupergiant.
Suggested fix
if (id==master) write(*,"(/,a,i2/)") '--> Testing ieos = ',ieos
call init_eos(ieos,ierr)
@@
- call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr')
+ if (do_radiation .and. (.not. eos_works_with_radiation(ieos))) then
+ expected_error = ierr_radiation_conflict
+ else
+ expected_error = 0
+ endif
+ call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr')
call update_test_scores(ntests,nfail,npass)
+ if (ierr == ierr_radiation_conflict) cycle📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (id==master) write(*,"(/,a,i2/)") '--> Testing ieos = ',ieos | |
| call init_eos(ieos,ierr) | |
| rmserr = 0. | |
| call set_star(id,master,star,xyzh,vxyzu,eos_vars,rad,& | |
| npart,npartoftype,massoftype,hfact,& | |
| xyzmh_ptmass,vxyz_ptmass,nptmass,ieos,gamma,X_in,Z_in,& | |
| relax=relax_star,use_var_comp=var_comp,write_rho_to_file=.false.,& | |
| rhozero=rhozero,npart_total=ntot,mask=i_belong,ierr=ierr,& | |
| write_files=.false.,density_error=rmserr,energy_error=ekin) | |
| call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr') | |
| call update_test_scores(ntests,nfail,npass) | |
| call check_setup(nerror,nwarn,restart=.false.) | |
| call checkval(nerror+nwarn,0,0,nfail(1),'no errors or warnings') | |
| call update_test_scores(ntests,nfail,npass) | |
| if (id==master) write(*,"(/,a,i2/)") '--> Testing ieos = ',ieos | |
| call init_eos(ieos,ierr) | |
| rmserr = 0. | |
| call set_star(id,master,star,xyzh,vxyzu,eos_vars,rad,& | |
| npart,npartoftype,massoftype,hfact,& | |
| xyzmh_ptmass,vxyz_ptmass,nptmass,ieos,gamma,X_in,Z_in,& | |
| relax=relax_star,use_var_comp=var_comp,write_rho_to_file=.false.,& | |
| rhozero=rhozero,npart_total=ntot,mask=i_belong,ierr=ierr,& | |
| write_files=.false.,density_error=rmserr,energy_error=ekin) | |
| if (do_radiation .and. (.not. eos_works_with_radiation(ieos))) then | |
| expected_error = ierr_radiation_conflict | |
| else | |
| expected_error = 0 | |
| endif | |
| call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr') | |
| call update_test_scores(ntests,nfail,npass) | |
| if (ierr == ierr_radiation_conflict) cycle | |
| call check_setup(nerror,nwarn,restart=.false.) | |
| call checkval(nerror+nwarn,0,0,nfail(1),'no errors or warnings') | |
| call update_test_scores(ntests,nfail,npass) |
🧰 Tools
🪛 Fortitude (0.9.0)
[error] 441-441: String uses single quotes but double quotes preferred
(S241)
[error] 452-452: trailing whitespace
(S101)
[error] 453-453: String uses single quotes but double quotes preferred
(S241)
[error] 457-457: 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/tests/test_setstar.f90` around lines 441 - 458, The variable
expected_error is used uninitialized in the checkval call within the set_star
test block, making the assertion nondeterministic. Initialize expected_error to
0 before calling init_eos. Additionally, add a radiation-conflict check after
init_eos similar to the pattern in test_redsupergiant that short-circuits and
exits the current iteration when radiation is enabled with an unsupported EOS,
preventing the set_star call and subsequent assertions from executing in those
cases.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tests/test_setstar.f90 (1)
441-454:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
expected_erroris used uninitialized, making this assertion nondeterministic.The variable
expected_errordeclared at line 400 is never assigned before use at line 453. In Fortran, local variables have undefined values unless explicitly initialized. This test should mirror the pattern intest_redsupergiant(lines 304-308).🐛 Proposed fix
if (id==master) write(*,"(/,a,i2/)") '--> Testing ieos = ',ieos call init_eos(ieos,ierr) rmserr = 0. call set_star(id,master,star,xyzh,vxyzu,eos_vars,rad,& npart,npartoftype,massoftype,hfact,& xyzmh_ptmass,vxyz_ptmass,nptmass,ieos,gamma,X_in,Z_in,& relax=relax_star,use_var_comp=var_comp,write_rho_to_file=.false.,& rhozero=rhozero,npart_total=ntot,mask=i_belong,ierr=ierr,& write_files=.false.,density_error=rmserr,energy_error=ekin) - + if (do_radiation .and. (.not. eos_works_with_radiation(ieos))) then + expected_error = ierr_radiation_conflict + else + expected_error = 0 + endif call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr') call update_test_scores(ntests,nfail,npass) + if (ierr == ierr_radiation_conflict) cycle🤖 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/tests/test_setstar.f90` around lines 441 - 454, The variable `expected_error` is used uninitialized in the checkval function call after the set_star invocation, which causes nondeterministic test behavior. Initialize `expected_error` to an appropriate value before calling checkval at line 453, following the same pattern used in the test_redsupergiant subroutine around lines 304-308. This ensures the test has a deterministic expected error value to compare against the actual ierr returned from set_star.
🤖 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.
Duplicate comments:
In `@src/tests/test_setstar.f90`:
- Around line 441-454: The variable `expected_error` is used uninitialized in
the checkval function call after the set_star invocation, which causes
nondeterministic test behavior. Initialize `expected_error` to an appropriate
value before calling checkval at line 453, following the same pattern used in
the test_redsupergiant subroutine around lines 304-308. This ensures the test
has a deterministic expected error value to compare against the actual ierr
returned from set_star.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 740a299d-bca0-468e-ba64-a0c1b0c7bbd8
📒 Files selected for processing (1)
src/tests/test_setstar.f90
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tests/test_setstar.f90 (1)
400-454:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
expected_erroris used uninitialized, causing nondeterministic test results.The variable
expected_erroris declared on line 400 but never assigned before use on line 453. Thetest_redsupergiantroutine (lines 304-311) demonstrates the correct pattern with radiation-conflict handling.Proposed fix
Update the
use eosstatement and add the conditional assignment:-use eos, only:init_eos,gamma,X_in,Z_in +use eos, only:init_eos,gamma,X_in,Z_in,eos_works_with_radiation -use setstar, only:star_t,set_star,set_defaults_star,imesa +use setstar, only:star_t,set_star,set_defaults_star,imesa,ierr_radiation_conflictThen before the
checkvalcall at line 453:write_files=.false.,density_error=rmserr,energy_error=ekin) - - call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr') + if (do_radiation .and. (.not. eos_works_with_radiation(ieos))) then + expected_error = ierr_radiation_conflict + else + expected_error = 0 + endif + call checkval(ierr,expected_error,0,nfail(1),'set_star runs with expected ierr') call update_test_scores(ntests,nfail,npass) + if (ierr == ierr_radiation_conflict) cycle🤖 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/tests/test_setstar.f90` around lines 400 - 454, The variable expected_error is declared on line 400 but never initialized before being used in the checkval call around line 453, causing nondeterministic test results. Following the pattern demonstrated in the test_redsupergiant routine (lines 304-311), add a conditional check before the checkval call to set expected_error appropriately based on radiation conflict detection, ensuring the variable is properly initialized with either 1 or 0 depending on whether radiation conflicts occur during the set_star call for each EOS type tested in the loop.
🧹 Nitpick comments (1)
src/tests/test_setstar.f90 (1)
425-431: 💤 Low valueRemove unused variables.
Several variables are declared/initialized but never used, likely copy-paste artifacts from
test_redsupergiant:
x0(line 425)ncheck,errmax(line 427)Zfrac(line 431)Proposed cleanup
-real :: rhozero,rmserr,ekin,x0(3),errmax(2) +real :: rhozero,rmserr,ekin real :: Mstar,tolprof,rhoj,rhoj_mesa,rj integer(kind=8) :: ntot -integer :: ierr,nfail(2),ncheck(2),i,j,nerror,nwarn,iunit,expected_error +integer :: ierr,nfail(1),i,j,nerror,nwarn,iunit,expected_error logical :: relax_star,var_comp -real, allocatable :: r(:),den(:),pres(:),temp(:),en(:),mtab(:),Xfrac(:),Yfrac(:),Zfrac(:),mu(:) +real, allocatable :: r(:),den(:),pres(:),temp(:),en(:),mtab(:),Xfrac(:),Yfrac(:),mu(:)And remove:
-x0 = 0. - -nfail = 0; ncheck = 0; errmax = 0. +nfail = 0call read_mesa(filepath,den,r,pres,mtab,en,temp,X_in,Z_in,Xfrac,Yfrac,mu,Mstar,ierr) -Zfrac = 1.-Xfrac-Yfrac🤖 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/tests/test_setstar.f90` around lines 425 - 431, Remove the unused variables that appear to be copy-paste artifacts from another test function. Delete the initialization of x0 on line 425, remove ncheck and errmax from the initialization statement on line 427 (keeping only nfail and tolprof), and delete the Zfrac assignment on line 431 since these variables are never referenced elsewhere in the test_setstar test function.
🤖 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.
Duplicate comments:
In `@src/tests/test_setstar.f90`:
- Around line 400-454: The variable expected_error is declared on line 400 but
never initialized before being used in the checkval call around line 453,
causing nondeterministic test results. Following the pattern demonstrated in the
test_redsupergiant routine (lines 304-311), add a conditional check before the
checkval call to set expected_error appropriately based on radiation conflict
detection, ensuring the variable is properly initialized with either 1 or 0
depending on whether radiation conflicts occur during the set_star call for each
EOS type tested in the loop.
---
Nitpick comments:
In `@src/tests/test_setstar.f90`:
- Around line 425-431: Remove the unused variables that appear to be copy-paste
artifacts from another test function. Delete the initialization of x0 on line
425, remove ncheck and errmax from the initialization statement on line 427
(keeping only nfail and tolprof), and delete the Zfrac assignment on line 431
since these variables are never referenced elsewhere in the test_setstar test
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6b9257e-c4a6-472b-aa35-7fcfa0794782
📒 Files selected for processing (1)
src/tests/test_setstar.f90
2 participants
Description:
A new module has been added: the zero temperature equation of state (see Kippenhahn 15.2). This is only considering degenerate pressure and internal energy. It is implemented mainly comparison purposes; the more accurate form of this is the Helmholtz EOS.
Components modified:
Type of change:
Testing:
Relaxed and tested two different WD masses. They matched expectations
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? maybe
Summary by CodeRabbit
Release Notes
New Features
ieos=25), including configurable species mass fractions and EOS calculations with reporting.Enhancements
data/star_data_filesand link it to the correct external dataset.Tests
ieos=15andieos=25, validating error metrics and skipping with a warning when the dataset is missing.