Prevented values requiring exponents of three digits from being written exactly in ASCII pixmaps.#129
Prevented values requiring exponents of three digits from being written exactly in ASCII pixmaps.#129s-neilson wants to merge 13 commits into
Conversation
…more digits are now truncated to values that can be formatted using an exponent of two digits. This prevents invalid values from being written to the file as ascii pixmaps are written with values that have exponents of two digits. The user is given warnings when the truncation happens.
… be represented with a negative exponent of two digits.
…mits for the largest and smallest values that can be represented with negative expodents of two digits.
|
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
ChangesPixmap ASCII clamping
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 value truncation and warning logs for extreme pixel values in the write_pixmap_ascii subroutine to prevent underflow and overflow issues. The feedback highlights two critical issues: first, declaring the temporary array on the stack can cause stack overflow for large pixmaps; second, the current implementation is highly inefficient as it performs nine passes over the array and uses single-precision literals that may cause compile-time errors. It is recommended to allocate the temporary array on the heap and optimize the truncation logic into a single-pass nested loop using double-precision literals.
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: 1
🤖 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/write_pixmap.f90`:
- Around line 167-185: The header in write_pixmap is being written from the
original datpix range before clamping, so it can advertise datmin/datmax values
that do not match the saved payload. Update the header-writing logic in
write_pixmap to compute and emit min/max from datpixTruncated after the
truncation and count adjustments, and ensure the same corrected values are used
anywhere else the file metadata is written so the .pix contents and header stay
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…he heap instead of the stack.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/write_pixmap.f90`:
- Around line 153-157: The current flow in write_pixmap opens the destination
file with status='replace' before allocating datpixTruncated, so an allocation
failure can delete an existing .pix file and exit without a valid replacement.
Move the datpixTruncated allocation and any copy/truncation work to occur before
the OPEN of the destination unit, or alternatively write to a temporary file and
only replace the final output after the allocation and write steps succeed. Keep
the fix localized around the datpixTruncated allocation/error path and the
destination file OPEN logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… can use the preprocessor.
… can use the preprocessor.
…uncation will only occur when SPLASH is compiled in double precision mode as it is unnecessary and creates problems in single precision mode. Also added truncation (only in double precision mode) to the header datmin and datmax values for the ascii and ppm pixmaps as they use the same formatting string as the pixel values.
|
Hi Stephen, thanks for highlighting + submitting a fix for this. However, strongly against introducing ifdefs into the codebase unless it is related to a library. Should rely on epsilon(0.) or tiny(0.) for finding tolerances in the working precision. |
|
@danieljprice I will find another solution that doesn't involve using the preprocessor. |
… name back to write_pixmap.f90). Also fixed missing references to sN,sP,lN and lP in write_pixmap_ascii.
|
@danieljprice I have changed it so write_pixmap will no longer use the preprocessor. |
2 participants
When ASCII pixmaps are written, they are written with values with exponents of two digits. If value is written that requires more digits, then the "E" will be missing from the written value, making the value invalid.
The following truncations are applied to data to be written into an ASCII pixmap:
The user is given warnings when the truncation happens. They are given the type of truncation that occurred and how many pixels that this was done to.
Similar truncation happens to the datmin and datmax values of the headers of ASCII and ppm pixmaps as they use the same formatting string as the ASCII pixmap data.
The truncation will only occur when SPLASH is compiled in double precision mode as it is unnecessary and causes problems when SPLASH is compiled in single precision mode.
I did it this way as opposed to changing the output format in order to maintain compatibility with any applications/scripts that expect a particular format.
Summary by CodeRabbit