Added user specified render bounds and temperature offset to lightcurve generation.#128
Added user specified render bounds and temperature offset to lightcurve generation.#128s-neilson wants to merge 7 commits into
Conversation
…xmin", "ymin", "xmax" and "ymax" flags. The render region bounds that are not specified are calculated automatically as they were before. The user can also now specify a constant tempature offset with the "o_col" flag.
|
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:
📝 WalkthroughWalkthrough
ChangesTemperature offset and render margin expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 temperature offset parameter o_col and allows custom rendering bounds via environment variables in src/lightcurve.f90. The review feedback highlights three key issues: a command-line flag collision where o_col conflicts with --fcol due to substring matching, a lack of validation for manual rendering bounds which could lead to division-by-zero errors if xmin >= xmax, and the potential for non-positive temperatures if a negative temperature offset is provided, which would cause failures in the Planck function calculation.
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: 3
🤖 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/lightcurve.f90`:
- Around line 136-139: The bounds setup in lightcurve handling now accepts
user-influenced margins, so the code must validate that each axis has xmax
greater than xmin before using the values to derive dx and npixy. Add a guard in
the lightcurve bounds calculation path around the xmin/xmax assignments (and the
later npixy computation site) to reject or correct invalid ordering for both
axes before any pixel geometry is computed.
- Around line 136-139: The bounds handling in lightcurve setup is treating
SPLASH_MARGIN_XMIN/XMAX/YMIN/YMAX as absolute replacements instead of margins.
Update the logic around the computed xmin/xmax values so the environment values
are applied as offsets relative to the auto-detected bounds, preserving the
existing margin semantics in the lightcurve rendering path.
- Line 250: The temperature offset in the lightcurve calculation can produce
non-positive values before the B_nu call, which then propagates invalid physics
through the outputs. Update the temperature computation in the lightcurve
routine around the tempi assignment to clamp or validate the result so tempi
always stays strictly positive before it is passed to B_nu, preserving the
existing flow in the same calculation block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… being less than or equal to the minimum bounds, and also prevented the temperature offset making particle temperatures less than or equal to zero. Lightcurve generation will be aborted in both of these scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lightcurve.f90`:
- Line 262: Fix the malformed conditionals in the lightcurve logic: the `if`
statement using `tempi` in the `src/lightcurve.f90` code currently splits the
relational operator, and the later check on `ierr` uses assignment syntax
instead of equality. Update the affected conditionals in the relevant lightcurve
routine so the comparisons use the proper Fortran operators `<=` and `==`,
keeping the surrounding control flow unchanged.
- Around line 235-237: The OpenMP loop in the lightcurve routine is writing to
the shared ierr variable, which can race across threads; change the parallel
logic around the loop in lightcurve.f90 to use a thread-local/reduction-style
flag and assign ierr = 6 only once after the parallel region completes. Also fix
the final conditional that checks ierr so it uses a comparison (ierr == 6)
rather than an assignment, and keep the change localized to the loop and
post-processing around the shared/private OpenMP clauses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… references to the spectral hardening factor, constant temperature offset and the user defined render region bounds.
1 participant
Added the following new features to lightcurve generation:
Also added references to the above new features to the lightcurve generation documentation. Also added a reference to the spectral hardening factor to the lightcurve generation documentation.
Summary by CodeRabbit
Summary of changes
New Features
ocolenvironment variable for non-sink particle light curves.SPLASH_MARGIN_{XMIN,XMAX,YMIN,YMAX}).Bug Fixes
Documentation
Style