Fix lightcurve generation: FITS error handling and relativistic NaNs#130
Fix lightcurve generation: FITS error handling and relativistic NaNs#130Dnsflr wants to merge 2 commits into
Conversation
…sure velocity columns are loaded for relativistic corrections
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR modifies two Fortran source files: ChangesLightcurve FITS and column fixes
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
There was a problem hiding this comment.
Code Review
This pull request updates src/analysis.f90 to set the required flags for velocity columns when ivx > 0, and modifies src/lightcurve.f90 to reset the error variable ierr to 0 after handling FITS write failures. The feedback suggests adding an upper-bound check on the required array slice in src/analysis.f90 to prevent potential 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/analysis.f90 (1)
206-206: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSame unguarded
required(ivx:...)pattern exists inenergy,rhomach, andkhcases.These cases set
required(ivx:ivx+ndimV-1) = .true.without theivx > 0guard added forlightcurve. Ifivx == 0for these analysis types, the same unintended spill intorequired(1)/required(2)described above would occur.♻️ Apply same guard to other cases
- required(ivx:ivx+ndimV-1) = .true. + if (ivx > 0) required(ivx:ivx+ndimV-1) = .true.(apply to lines 206, 321, 336)
Also applies to: 321-321, 336-336
🤖 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/analysis.f90` at line 206, The same unguarded required(ivx:ivx+ndimV-1) assignment used in the energy, rhomach, and kh branches of analysis.f90 must also be protected with the ivx > 0 check added for lightcurve. Update the matching cases in the analysis logic so they only mark required(ivx:...) when ivx is valid, using the same guard pattern around the existing required slice assignment in each branch.
🤖 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.
Nitpick comments:
In `@src/analysis.f90`:
- Line 206: The same unguarded required(ivx:ivx+ndimV-1) assignment used in the
energy, rhomach, and kh branches of analysis.f90 must also be protected with the
ivx > 0 check added for lightcurve. Update the matching cases in the analysis
logic so they only mark required(ivx:...) when ivx is valid, using the same
guard pattern around the existing required slice assignment in each branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cf88432-62cd-4fd4-b8a5-f03cd292432e
📒 Files selected for processing (2)
src/analysis.f90src/lightcurve.f90
…d velocity columns
Fix lightcurve generation: FITS error handling and relativistic NaNs
This PR addresses two bugs encountered when generating lightcurves across multiple dump files using
splash calc lightcurve:1. Empty
lightcurve.outon FITS export failureget_lightcurve(e.g. throwingERROR: mom0 FITS image not written; skipping cube), the subroutine returns withierr /= 0. This causes the conditionif (ierr == 0)insrc/analysis.f90to evaluate to false, skipping the writing of data tolightcurve.outentirely.ierrvariable back to0inside the error catching blocks for bothwrite_fits_imageandwrite_fits_cubeinsrc/lightcurve.f90. This allows the FITS export to gracefully fail while the subroutine still returns a successful code, ensuring the main data is still properly written tolightcurve.out.2. NaN output in relativistic calculations during multi-file processing
NaNvalues forL_bolandT_effon subsequent files. Because sequential file analysis uses a partial read mechanism, the velocity vector components (vx, vy, vz) were omitted from therequiredarray in thecase('lightcurve')initialization block. The first dump file bypasses this issue (as it reads all columns to establish the header), but subsequent files skip loading velocity data. This leads to uninitialized memory being passed to theget_lightcurvesubroutine, resulting in NaNs when calculating the Lorentz factor (doppler_factor_max).case('lightcurve')block insidesrc/analysis.f90:Summary by CodeRabbit