HDF5 output for Krome analysis#853
Conversation
- adapt number of threads to 1 if Krome is compiled with gfortran
allow for preprocessing of the file
…atasets with particle array
…g it, fix gfortran compilation failure, change rundir to CI workspace
allow for preprocessing of the file
…into Krome-hdf5
…atasets with particle array
…into Krome-hdf5
…g it, fix gfortran compilation failure, change rundir to CI workspace
…into Krome-hdf5
Krome output in hdf5 format
|
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 (2)
📝 WalkthroughWalkthroughThe PR adds KROME chemistry analysis with HDF5 persistence, introduces directional ray-tracing controls, updates KROME build and CI configuration, documents the KROME workflow, and includes formatting-only changes across EOS, setup, tests, and contributor metadata. ChangesKROME chemistry integration
Maintenance formatting updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Analysis as do_analysis
participant RayTracer as get_all_tau
participant KROME
participant HDF5
Analysis->>HDF5: Read or initialize chemistry state
Analysis->>RayTracer: Compute column density
Analysis->>KROME: Set inputs and evolve abundances
Analysis->>HDF5: Write abundances and species labels
Possibly related PRs
🚥 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 |
![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 KROME-based chemical analysis module (analysis_krome.F90) with HDF5 support, updates the build system, and adds directional ray-tracing capabilities to the raytracer. Feedback on these changes highlights several critical issues: in the new analysis module, there is a dataset name mismatch between reading and writing, a potential stack overflow from large stack-allocated arrays, an O(N^2) performance bottleneck in particle matching, and compilation errors when OpenMP is disabled. Additionally, a regression in the raytracer was identified where removing non-SPH particle checks could lead to out-of-bounds array access.
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/utils_raytracer.f90 (1)
487-529: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the gas-only bounds check here
find_nextindexesxyzhandkappadirectly fromlistneigh, and the same tree infrastructure can yield indices abovemaxpsphin sink-tree mode (other callers filter those out). Without a guard, this can read past the SPH arrays.🤖 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/utils_raytracer.f90` around lines 487 - 529, In find_next, guard each listneigh(i) access before indexing xyzh or kappa so only gas/SPH indices within the valid maxpsph bound are processed. Skip out-of-range sink-tree entries entirely, preserving the existing optical-depth accumulation and nearest-ray-particle selection for valid neighbours.
🧹 Nitpick comments (3)
src/utils/analysis_krome.F90 (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModule is missing a default-accessibility
privatestatement.Without an explicit
privateafterimplicit none, every module-level variable (abundance,abundance_prev,iorig_old,iprev,done_init,mask,AuvAv,albedo,abundance_label) is implicitly public, exposing internal state beyond the intendeddo_analysisAPI surface.♻️ Suggested fix
implicit none character(len=20), parameter, public :: analysistype = 'krome' public :: do_analysis +private logical, allocatable :: mask(:)🤖 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/utils/analysis_krome.F90` around lines 26 - 28, Add a default module accessibility declaration of private immediately after implicit none in the analysis_krome module, while retaining explicit public access for analysistype and do_analysis. This should keep all other module-level state symbols private by default.Source: Linters/SAST tools
src/main/utils_raytracer.f90 (1)
384-391: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
outwardsdeclared optional withoutintent, dereferenced withoutpresent()check.
outwardsis declaredlogical, optional(nointent) yetray_tracerdoesif (.not. outwards) thenunconditionally. Every current call site happens to pass it, but the combination of "optional" + missingintent(in)+ nopresent()guard is a latent crash/undefined-behavior trap if any future caller omits it — and it deviates from the existingmaxDistanceoptional handling pattern (which is checked viapresent()).♻️ Suggested fix
-real, intent(in) :: primary(3), ray(3), Rstar, Rinject, xyzh(:,:), kappa(:) -logical, optional :: outwards -real, optional :: maxDistance +real, intent(in) :: primary(3), ray(3), Rstar, Rinject, xyzh(:,:), kappa(:) +logical, intent(in) :: outwards +real, intent(in), optional :: maxDistanceAlso applies to: 429-432
🤖 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/utils_raytracer.f90` around lines 384 - 391, Update the outwards argument declaration in ray_tracer to use intent(in), and guard every access to it with present(outwards), preserving the existing default behavior when callers omit the optional argument. Apply the same protection to the related logic around the additional referenced block, while leaving maxDistance handling unchanged.Source: Linters/SAST tools
build/Makefile (1)
888-888: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
analysis_krome.F90arm on case-sensitive filesystems —$(wildcard ../src/utils/analysis_*.f90)never includesanalysis_krome.F90, so thiscontinuebranch is unreachable on Linux. Either drop the uppercase arm or widen the glob to include.F90if the exclusion should be explicit.🤖 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 `@build/Makefile` at line 888, Update the Makefile wildcard and matching case arm so the `analysis_krome.F90` exclusion is reachable on case-sensitive filesystems: either remove the uppercase case arm, or widen the wildcard to include `.F90` while preserving the intended exclusion 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.
Inline comments:
In @.github/workflows/krome.yml:
- Around line 34-42: Update the Ubuntu “Install HDF5 dependencies” step in
.github/workflows/krome.yml (lines 34-42) to export HDF5INCLUDE as
/usr/include/hdf5/serial and HDF5LIB as /usr/lib/x86_64-linux-gnu/hdf5/serial
instead of relying on HDF5_DIR-derived paths. The build/Makefile
HDF5INCLUDE/HDF5LIB fallback at lines 396-408 requires no direct change.
In `@src/utils/analysis_krome.F90`:
- Line 60: Remove the inline initializers from the local declaration in
do_analysis for isize, npart_copy, and i_radius, then explicitly assign their
intended initial values at the start of each invocation before use. Keep the
existing values of 0, 0, and 1 respectively without introducing implicit SAVE
state.
- Around line 243-329: Update the dataset opened by read_chem to use the same
"abundances" name created by write_chem, preserving the restart data path. In
the read_chem HDF5 failure branches, replace silent returns that leave abundance
zeroed with the established fatal error handling or the documented per-particle
chem_init fallback, ensuring failures cannot continue with invalid zero
abundances.
- Around line 154-167: Update the particle matching loop in the outer/inner do
blocks to use an explicit logical found flag initialized false for each
particle, set true when iorig matches and iprev(i) is assigned, then select
abundance_prev only when found is true; otherwise call chem_init. Remove the
condition’s dependence on the post-loop value of j.
- Line 58: Update the declarations in the analysis routine containing
abundance_part, Y, column_density, and xyzh_copy so the npart-sized
column_density and xyzh_copy buffers are allocatable or stored in persistent
work storage rather than automatic stack arrays. Add the necessary allocation
and cleanup or reuse logic while preserving their existing dimensions and use.
- Around line 118-129: Avoid permanently overwriting xyzmh_ptmass(iReff,1) in
the non-first-step branch before build_tree. Use a local sink-radius copy when
invoking get_all_tau, or save and restore the original value afterward; preserve
the intended temporary value without clobbering shared sink state.
- Around line 154-190: Remove the hardcoded i > 2460 condition from the KROME
processing guard in the outer loop, leaving iamtype(iphase(i)) /= iboundary as
the sole boundary check. Ensure every non-boundary particle reaches the
thermodynamic/radiation calculations and updates abundance(:,i) =
abundance_part.
---
Outside diff comments:
In `@src/main/utils_raytracer.f90`:
- Around line 487-529: In find_next, guard each listneigh(i) access before
indexing xyzh or kappa so only gas/SPH indices within the valid maxpsph bound
are processed. Skip out-of-range sink-tree entries entirely, preserving the
existing optical-depth accumulation and nearest-ray-particle selection for valid
neighbours.
---
Nitpick comments:
In `@build/Makefile`:
- Line 888: Update the Makefile wildcard and matching case arm so the
`analysis_krome.F90` exclusion is reachable on case-sensitive filesystems:
either remove the uppercase case arm, or widen the wildcard to include `.F90`
while preserving the intended exclusion behavior.
In `@src/main/utils_raytracer.f90`:
- Around line 384-391: Update the outwards argument declaration in ray_tracer to
use intent(in), and guard every access to it with present(outwards), preserving
the existing default behavior when callers omit the optional argument. Apply the
same protection to the related logic around the additional referenced block,
while leaving maxDistance handling unchanged.
In `@src/utils/analysis_krome.F90`:
- Around line 26-28: Add a default module accessibility declaration of private
immediately after implicit none in the analysis_krome module, while retaining
explicit public access for analysistype and do_analysis. This should keep all
other module-level state symbols private by default.
🪄 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: a897f355-dfe0-45e3-b95c-8b983272001a
📒 Files selected for processing (11)
.github/workflows/krome.ymlAUTHORSbuild/Makefilesrc/main/eos.f90src/main/eos_zerotemp.f90src/main/ptmass_radiation.f90src/main/utils_raytracer.f90src/setup/setup_grtde.f90src/tests/test_setstar.f90src/utils/analysis_krome.F90src/utils/analysis_krome.f90
💤 Files with no reviewable changes (1)
- src/utils/analysis_krome.f90
…into Krome-hdf5
…into Krome-hdf5
documentation for Krome
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/utils/analysis_krome.F90 (5)
283-309: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
write_chem: several HDF5 calls have uncheckedhdferr.
h5gcreate_f(283),h5screate_simple_f(287, 299),h5tcopy_f/h5tset_size_f(300, 302), andh5dcreate_f/h5dwrite_fforspecies_labels(303-304) don't checkhdferr. A failure partway through (e.g. disk full) would leave a partially-written, seemingly-valid.h5file that later silently feeds bad/zeroed data intoread_chem.🤖 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/utils/analysis_krome.F90` around lines 283 - 309, Update write_chem to check hdferr immediately after every listed HDF5 operation: h5gcreate_f, both h5screate_simple_f calls, h5tcopy_f, h5tset_size_f, and the species_labels h5dcreate_f/h5dwrite_f calls. On any failure, report the write error and exit through cleanup that closes only successfully created HDF5 resources, preventing a partially written file from being treated as valid.
164-186: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winStale
iprevacross calls can bypass the match search and silently reuse the wrong particle's abundances.
iprevis a module-level array only zeroed once, on the very first call (line 97). On later calls,if (iprev(i) == 0)(line 172) is used to decide whether to run the inner search (173-180) — but ifiprev(i)still holds a nonzero value left over from an earlier call (because the fast-path check at 166-171 didn't match this call), the inner search is skipped entirely, andj(private, never assigned this iteration) is compared at line 182 against that staleiprev(i). If they coincidentally match,abundance_prev(:,iprev(i))— the wrong particle's chemistry state — is silently applied instead of callingchem_init. This can inject silently-corrupted abundances into the simulation with no error indication.🐛 Proposed fix
outer: do i=1,npart if (.not.isdead_or_accreted(xyzh(4,i))) then + found = .false. + iprev(i) = 0 if (i <= nprev) then if (iorig(i) == iorig_old(i)) then iprev(i) = i - j = i + found = .true. endif endif if (iprev(i) == 0) then inner: do k=1,nprev if (iorig(i) == iorig_old(k)) then iprev(i) = k - j = k + found = .true. exit inner endif enddo inner endif - if (j == iprev(i)) then + if (found) then abundance_part(:) = abundance_prev(:,iprev(i)) else call chem_init(abundance_part) endif(add
logical :: foundto the declarations andfoundto theprivate()clause)🤖 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/utils/analysis_krome.F90` around lines 164 - 186, Update the particle matching loop around outer so each iteration resets matching state and cannot reuse stale module-level iprev values. Add the proposed logical found variable to the declarations and private clause, use it to track whether the current particle matched, and ensure the inner search runs when the fast path fails; only copy abundance_prev when a current-call match is found, otherwise call chem_init.
41-61: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBroken dummy-argument declaration will not compile:
xzymh_ptmass_copyvsxyzmh_ptmass_copy.
xzymh_ptmass_copy(:,:)is declared asintent(in)even though it is not in the subroutine argument list, and the laterxyzmh_ptmass_copyreferences are undeclared underimplicit none. This needs one consistent local declaration or a real dummy argument in the signature.🤖 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/utils/analysis_krome.F90` around lines 41 - 61, Fix the dummy-argument declaration in do_analysis by consistently using the existing xyzmh_ptmass argument name: remove the undeclared xzymh_ptmass_copy declaration and either declare xyzmh_ptmass_copy as the intended dummy argument in the signature or define it as a correctly named local variable, ensuring all later references compile under implicit none.
158-216: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
kto the OpenMP private clause. The nesteddo k=1,nprevruns inside the parallel region, anddefault(none)requireskto be explicitly scoped here.🤖 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/utils/analysis_krome.F90` around lines 158 - 216, Add loop variable k to the private clause of the OpenMP parallel directive for the outer loop, alongside the existing private variables, so the nested do k=1,nprev uses thread-local storage under default(none).
313-371: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOpen the
abundancesdataset before callingh5dget_space_f.dset_idis used here before anyh5dopen_f(..., 'abundances', dset_id, ...), so the restart read can hit undefined behavior or crash.🤖 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/utils/analysis_krome.F90` around lines 313 - 371, Open the `abundances` dataset within `read_chem` using `h5dopen_f` on `group_id` before calling `h5dget_space_f` or reading its dimensions/data. Check the open result and preserve the existing dataset close and shape-validation flow.
🤖 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 `@docs/physics/composition.rst`:
- Around line 166-171: Correct the documented location of analysis_krome.F90 in
the composition instructions from src/analysis/ to src/utils/analysis_krome.F90,
while preserving the existing guidance for editing set_initial_abundances and
recompiling phantomanalysis.
- Around line 104-107: Update the HDF5 environment-variable instructions in the
composition documentation to remove the author-specific /home/camille paths. Set
HDF5INCLUDE and HDF5LIB using the user’s HDF5 installation prefix while
preserving the existing include and library subdirectories expected by
build/Makefile.
- Around line 46-60: In the HDF5 installation instructions, replace both `h5cc
-showconfig` references with `h5fc -showconfig`, including the introductory
check and the gfortran installation verification, so they use the Fortran
compiler path.
In `@src/utils/analysis_krome.F90`:
- Around line 63-72: Update the non-OpenMP branches around the thread-status
print logic so they do not call omp_get_max_threads() unless _OPENMP is defined;
use a plain serial-status message for those branches while preserving the
existing OpenMP behavior.
---
Outside diff comments:
In `@src/utils/analysis_krome.F90`:
- Around line 283-309: Update write_chem to check hdferr immediately after every
listed HDF5 operation: h5gcreate_f, both h5screate_simple_f calls, h5tcopy_f,
h5tset_size_f, and the species_labels h5dcreate_f/h5dwrite_f calls. On any
failure, report the write error and exit through cleanup that closes only
successfully created HDF5 resources, preventing a partially written file from
being treated as valid.
- Around line 164-186: Update the particle matching loop around outer so each
iteration resets matching state and cannot reuse stale module-level iprev
values. Add the proposed logical found variable to the declarations and private
clause, use it to track whether the current particle matched, and ensure the
inner search runs when the fast path fails; only copy abundance_prev when a
current-call match is found, otherwise call chem_init.
- Around line 41-61: Fix the dummy-argument declaration in do_analysis by
consistently using the existing xyzmh_ptmass argument name: remove the
undeclared xzymh_ptmass_copy declaration and either declare xyzmh_ptmass_copy as
the intended dummy argument in the signature or define it as a correctly named
local variable, ensuring all later references compile under implicit none.
- Around line 158-216: Add loop variable k to the private clause of the OpenMP
parallel directive for the outer loop, alongside the existing private variables,
so the nested do k=1,nprev uses thread-local storage under default(none).
- Around line 313-371: Open the `abundances` dataset within `read_chem` using
`h5dopen_f` on `group_id` before calling `h5dget_space_f` or reading its
dimensions/data. Check the open result and preserve the existing dataset close
and shape-validation flow.
🪄 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: c91448f0-d11d-409e-9996-ed1ce596e44d
📒 Files selected for processing (2)
docs/physics/composition.rstsrc/utils/analysis_krome.F90
|
@camillelandri thanks for this P-R! I fixed the mcfost problem on main... |
|
seems like there is one "real" failure left, which is something to do with the raytracer api in analysis_raytracer.f90 merge of main->branch should solve the mcfost issue |
|
only "real" error here is in batch 23, (e.g. make SETUP=radwind analysis): |
2 participants
Description:
Analysis routines with Krome now produce hdf5 output files for the chemical abundances instead of ASCII files.
The routine can read abundance files to initialise Krome, thus making the post-processing routine restartable.
Phantom is automatically compiled with HDF5 if KROME=yes, and HDF5DIR must be provided (as stated in the documentation for HDF5 routines in phantom). The CI was modified accordingly.
Some changes to the Mats' ray tracer were included, since he didn't push them to the main repo and they're needed for Krome to work properly.
Components modified:
Type of change:
Testing:
Tested on MacOS and Linux - ran the analysis on a typical wind model, timed the writing routines to ensure they were optimal (writing 468 abundances * 3million takes a while...)
Did you run the bots? yes
Did you update relevant documentation in the docs directory? no (in progress)
Did you add comments such that the purpose of the code is understandable? yes
Summary by CodeRabbit