Aton reader module added (similar to MESA reader) - #858
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds ATON stellar-profile file parsing and validation, registers an ATON profile option, integrates it into stellar setup, relaxation, softened-core handling, and EOS configuration, and includes the source in star-related builds. ChangesATON Profile Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant setstar
participant read_star_profile
participant read_aton
participant write_aton
setstar->>read_star_profile: select iaton profile
read_star_profile->>read_aton: load input profile
read_aton-->>read_star_profile: profile data and ierr
read_star_profile->>write_aton: write softened ATON profile
write_aton-->>read_star_profile: softened profile
read_star_profile->>read_aton: reload softened profile
read_aton-->>read_star_profile: profile data and ierr
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (2)
src/.vscode/settings.json (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop this local editor config from the PR.
src/.vscode/settings.jsondisables the Fortran language server for every contributor and is unrelated to ATON support. Keep it local (or gitignored).🤖 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/.vscode/settings.json` around lines 1 - 3, Remove the src/.vscode/settings.json editor configuration from the change so the fortran.fortls.disabled setting is not committed. Keep this local-only configuration outside version control or ensure it is gitignored.src/setup/readwrite_aton.f90 (1)
63-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover debug prints.
Lines 64, 73, 88 (and arguably 97) are development traces (
'I am after ...') that will pollute setup output.🧹 Proposed cleanup
lines = get_nlines(fullfilepath) ! total number of lines in file -write(*,*)'I am after get_nlines', lines open(newunit=iu,file=fullfilepath,status='old',iostat=ierr) @@ call get_ncolumns(iu,ncols,nheaderlines) - write(*,*)'I am after get_ncolummns in ATON. This is nheaderlines', nheaderlines lines = lines - nheaderlines @@ call read_column_labels(iu,nheaderlines,ncols,nlabels,header) - write(*,*)'I am after read_column_labels', nlabels,ncols🤖 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/setup/readwrite_aton.f90` around lines 63 - 97, Remove the development trace write statements around get_nlines, get_ncolumns, and read_column_labels in the setup flow; retain the meaningful “Reading ATON from centre to surface” status message unless it is also considered unwanted debug output.
🤖 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/setup/readwrite_aton.f90`:
- Around line 179-184: Update the ATON input validation in the read routine to
include the initialized sentinel arrays r and m alongside pres and rho, so
missing radius or mass columns set an error before downstream calculations. Use
a distinct ierr value for missing required columns, and ensure the setstar_utils
error mapping reports incomplete columns rather than “file does not exist,”
while preserving the existing missing-file code.
- Around line 254-262: Update read_aton’s mass-unit detection to inspect the
parsed column-label line for the case-insensitive `#m/msun` label, rather than
relying on nheaderlines == 5. Ensure the ATON-specific solar-mass conversion is
enabled whenever that label is present before cgs conversion.
- Line 17: Update the :Dependencies: header for read_aton to include table_utils
alongside the existing dependencies, matching its use of table_utils:flip_array.
Leave the implementation unchanged.
- Around line 170-177: Remove the dead rewind-and-header-reskip block after the
data-reading logic so it cannot overwrite ierr before the existing validation.
Also remove the vestigial idir declaration, conditional print, and related
scaffolding in the readwrite_aton routine, while preserving the actual
header/data parsing and close behavior.
In `@src/setup/readwrite_mesa.f90`:
- Line 13: Correct the header metadata tag in the file header from :OWner: to
:Owner:, preserving the existing owner value and leaving unrelated ATON changes
untouched.
In `@src/setup/set_star.f90`:
- Line 1180: Wrap the long conditional call in the setup flow around the
`read_inopt` invocation, splitting the `any(star(:)%iprofile==imesa .or.
star(:)%iprofile==iaton)` condition across continuation lines so every line
stays within 100 columns. Apply the same formatting to the corresponding
condition around the second occurrence noted in the review, without changing
behavior.
---
Nitpick comments:
In `@src/.vscode/settings.json`:
- Around line 1-3: Remove the src/.vscode/settings.json editor configuration
from the change so the fortran.fortls.disabled setting is not committed. Keep
this local-only configuration outside version control or ensure it is
gitignored.
In `@src/setup/readwrite_aton.f90`:
- Around line 63-97: Remove the development trace write statements around
get_nlines, get_ncolumns, and read_column_labels in the setup flow; retain the
meaningful “Reading ATON from centre to surface” status message unless it is
also considered unwanted debug output.
🪄 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: 4b67ce54-6b64-4a0f-bce2-5fc34247cb88
📒 Files selected for processing (6)
build/Makefilesrc/.vscode/settings.jsonsrc/setup/readwrite_aton.f90src/setup/readwrite_mesa.f90src/setup/set_star.f90src/setup/set_star_utils.f90
|
Hello Daniel, I have addressed all the bot comments that came to me after my pull request. I have tested and re-committed and also pushed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/setup/set_star.f90 (1)
676-678: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an end-to-end regression test for profile option 8.
Exercise
phantomsetupwithSETUP=star,ieos=10, and ATON option 8, then assert that a dump is created. This validates the parser, option registration, EOS configuration, and softened-profile path wired throughsrc/setup/set_star_utils.f90.🤖 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/setup/set_star.f90` around lines 676 - 678, Add an end-to-end regression test invoking phantomsetup with SETUP=star, ieos=10, and ATON profile option 8, then assert that the expected dump file is created. Ensure the test exercises the parser, option registration, EOS setup, and softened-profile path in set_star_utils.
🤖 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/setup/readwrite_aton.f90`:
- Line 17: Update the dependency metadata comment in the module header so the
entries use canonical alphabetical order, placing table_utils before units while
preserving all other dependencies.
- Around line 81-84: Format the label scan in the isatonfile detection block to
satisfy Fortitude: split the long conditional involving header(i) across
continuation lines and use the project’s lint-compliant loop terminator form for
both affected loops, including the loop near the second reported location.
In `@src/setup/set_star.f90`:
- Around line 1180-1181: Convert the continued inline if statements guarding
read_inopt calls in the setup flow to block if/end if statements, including both
occurrences near the star profile checks. Preserve the existing conditions, call
arguments, and execution behavior while making the EOS wiring explicit.
---
Nitpick comments:
In `@src/setup/set_star.f90`:
- Around line 676-678: Add an end-to-end regression test invoking phantomsetup
with SETUP=star, ieos=10, and ATON profile option 8, then assert that the
expected dump file is created. Ensure the test exercises the parser, option
registration, EOS setup, and softened-profile path in set_star_utils.
🪄 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: dd72345e-255e-4dc0-af77-1a9c925d5326
📒 Files selected for processing (2)
src/setup/readwrite_aton.f90src/setup/set_star.f90
|
One more round of minor fixes identified by the bot. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/setup/readwrite_aton.f90 (1)
73-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose
iubefore every post-open return.
read_atonopensiuat Line 65, but the empty-data, data-read-error, and invalid-header branches return without closing it. Repeated calls can leak Fortran I/O units; use a cleanup path or close the unit before each return.Proposed fix
if (lines <= 0) then ierr = 1 + close(iu) return endif ... if (ierr /= 0) then print "(a,/)",' ERROR reading data from ATON file (new statement)' + close(iu) return endif ... ierr = 2 + close(iu) returnAlso applies to: 94-99, 106-112
🤖 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/setup/readwrite_aton.f90` around lines 73 - 75, Update read_aton so every return after opening iu closes the unit first, including the empty-data, data-read-error, and invalid-header branches. Prefer a shared cleanup path if appropriate, while preserving each branch’s existing ierr behavior.
🤖 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.
Outside diff comments:
In `@src/setup/readwrite_aton.f90`:
- Around line 73-75: Update read_aton so every return after opening iu closes
the unit first, including the empty-data, data-read-error, and invalid-header
branches. Prefer a shared cleanup path if appropriate, while preserving each
branch’s existing ierr behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4d338cc-6789-4971-b6a7-4a7b79170979
📒 Files selected for processing (2)
src/setup/readwrite_aton.f90src/setup/set_star.f90
|
Delighted to see your pull request @orsolademarco-astronomy :) One thing that would be helpful for testing is to supply an example Aton file that we can use as the default setup for iprofile=8. You should be able to just attach it as a file in the description or comments thread, or alternatively post an example file on zenodo and link to the phantom zenodo community (and update src/main/datafiles.f90 with the lookup address). One of the tests we run is to check that default SETUP=star with iprofile=[0,1,2,3,4,5...] etc all manage to relax a star with default settings. This is done by some code in scripts/buildbot.sh: It'd make sense to expand the range to 8... that way we can guarantee that the ATON read is not broken. There are also a bunch of tests run in .github/workflows/binary.yml that test against various "known" .setup and .in files so could equivalently add something there (e.g. if you want to test something other than a single star) |
|
I have a question about updating datafiles.f90. I expected to have to add the new zenodo repo, but what I found was there is only one line referring to data/star_data_files which currently points to a zenodo repo that Ali has added with his WD file. (I realise that a number of other star data files are inside the phantom repo, like the Kpler or Jan stars.) If I just want to add my new zendo repo with the ATON star what case should I use since "my case" is already used? This is what is in the file: case('data/star_data_files') but I also need the file to go into data/star_data_files, but I cannot make a new case, nor have another URL in there. The AI suggestion is a structural rewrite of datafiles.f90 to be more granular and search files rather than entire directories, but I do not really want to go there. So the alternative you give is the zenodo link to phantom which I have done. But the other suggestions you make "You should be able to just attach it as a file in the description or comments thread" seems vague. Should I not just commit the file to the data_star_file for now? Awaiting a rationalisation of star files in general? |
|
I will merge this for now, I can add the test in a subsequent P-R, the file is online so should be easy. General documentation on datafiles here: |
2 participants
Description:
Added a readwrite_aton module similar to MESA and KEPLER. Updated all the lines in setup that needed to include the new option.
Components modified:
Type of change:
Testing:
I can read the file which is now option 8, and the behaviour is identical to mesa.
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 I think anything that is done for MESA would work.
If so, please describe what a unit test might check:
a test should check that running phantomsetup on SETUP=star with ieos=10 and option=8 successfully creates a dump
Summary by CodeRabbit
New Features
Bug Fixes
Chores