Fix 6 safe pre/post-process bugs (batch)

[sbryngelson]

User description

Summary

Batches 6 low-risk bug fixes in pre-process, post-process, and common CPU code paths. These have no GPU kernel or MPI hot-path impact, making them safe to merge with GitHub CI alone.

Included fixes

• s_create_directory wildcard (Fix s_create_directory called with /* wildcard creating junk directory #1214): mkdir was called with /* glob, creating a literal * directory
• Integral z-bounds init (Fix integral zmin/zmax never initialized (copy-paste of ymin/ymax) #1177): zmin/zmax never initialized (copy-paste of ymin/ymax)
• OBJ reader index (Fix OBJ reader using j as both triangle counter and vertex index #1178): j used as both triangle counter and vertex index
• time_real init (Fix time_real used uninitialized in Lagrangian bubble output #1189): Used uninitialized in Lagrangian bubble post-process output
• s_write_intf_data_file loop index (Fix s_write_intf_data_file using wrong loop index for distance check #1191): Wrong loop index for distance check in post-process
• Simplex noise mod range (Fix simplex noise using mod(i,255) instead of iand(i,255) #1218): mod(i,255) skips index 255; fixed to iand(i,255)

Supersedes

Closes #1214, closes #1177, closes #1178, closes #1189, closes #1191, closes #1218, fixes #1196, fixes #1199, fixes #1210, fixes #1212, fixes #1220, fixes #1223

Test plan

• All GitHub CI checks pass (ubuntu MPI debug, ubuntu no-mpi, macOS)
• No Fortran source files in simulation hot path are touched
• Each fix is a 1-3 line change in pre-process or post-process only

🤖 Generated with Claude Code

---

CodeAnt-AI Description

Fix six pre/post-process bugs that produced incorrect files, geometry, and output values

What Changed

• Directory cleanup no longer creates a literal '*' folder; processor directories are deleted correctly before creating timestep folders
• 3D integral probe z-bounds are initialized properly so volume integrals use valid zmin/zmax values
• OBJ reader now uses the correct third vertex index so imported meshes have accurate triangle geometry
• Particle/bubble output time column is set from the file header so printed times are correct instead of garbage
• Interface-point selection avoids comparing against a stale index so nearby point detection and spacing work as intended
• Simplex noise index wrapping uses bitwise mask so the full intended index range is covered

Impact

✅ No stray '*' directories after pre-process
✅ Correct 3D integral results from valid z-bounds
✅ Accurate mesh geometry when importing OBJ files

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[codeant-ai]

Nitpicks 🔍

🔒 No security issues identified

⚡ Recommended areas for review Index validation After reading face indices (k, l, iv3) there is no validation or handling of negative indices (OBJ allows negative indices) or out-of-range values. This can cause out-of-bounds access on vertices(...) and crash or corrupt memory at runtime. Add bounds checks and negative-index handling. OBJ parsing The new OBJ face read uses a simple list read (three integers) which will fail or produce incorrect indices for common OBJ face formats that include texture/normal indices (e.g. "f v/vt/vn") or for quad faces. The reader should be hardened to accept or reject these formats explicitly and/or parse tokens to extract vertex indices only. Directory Deletion The code now calls s_delete_directory(trim(proc_rank_dir)) (removed the literal '*' bug). Verify that s_delete_directory's semantics are correct here: it should remove only contents (or handle a missing directory) and must not unintentionally remove parent directories needed later. Also ensure this is safe in parallel runs (no race conditions from multiple processes hitting the same filesystem path) and that failures (nonexistent path, permissions) are handled gracefully. Permutation mask The change from mod(i,255) to iand(i,255) fixes skipping index 255 by using a bitmask. Verify that input indices i/j are non-negative or that negative values are handled as intended; also confirm integer kind/magnitude matches the expectations of p_vec indexing. Dedup logic change The duplicate-point detection in s_write_intf_data_file was reworked: the per-point Euclidean distance is now computed inside the loop and the code uses exit when a close point is found. Confirm the new behavior exactly matches the intended semantics (do not add a new point if any existing point lies within tgp). Also check loop ordering and index usage (x/y indices) to ensure comparisons use the correct coordinate arrays and the counter logic is robust.

[cubic-dev-ai]

No issues found across 5 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

[Copilot]

Pull request overview

Batches several small correctness fixes across MFC utilities and I/O paths, targeting directory handling, initialization bugs, indexing errors, and post-process output correctness.

Changes:

• Fixes directory cleanup logic in serial pre-process so it deletes the intended rank directory instead of using a non-expanding wildcard.
• Corrects initialization/indexing issues (integral z-bounds init, OBJ face parsing index handling).
• Fixes post-process outputs (initialize time_real from restart metadata; correct interface point distance checking) and simplex-noise index wrapping.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/simulation/m_global_parameters.fpp	Initializes volume-integral zmin/zmax correctly (instead of mistakenly repeating y-bounds).
src/pre_process/m_start_up.fpp	Replaces wildcard-based directory “cleanup” with deletion of the actual per-rank directory, then recreates /0.
src/pre_process/m_simplex_noise.fpp	Uses iand(…,255) for correct 0–255 permutation indexing in 2D simplex noise.
src/post_process/m_data_output.fpp	Initializes time_real from file_time and fixes the interface-point distance check loop to use the correct index and early-exit logic.
src/common/m_model.fpp	Fixes OBJ face parsing so the third vertex index doesn’t overwrite the triangle counter (j).

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/post_process/m_data_output.fpp`:
- Line 1271: The assignment time_real = file_time in the Silo output function is
dead code because time_real is never used; either remove that assignment from
the Silo branch or add the simulation time metadata to the DBPUTPM call so Silo
stores time. Locate the Silo variant of the output routine (the code path that
calls DBPUTPM) and either delete the time_real variable and its assignment, or
build an options list containing the time (e.g., set "time" or "cycle" entries
using file_time/time_real) and pass it into DBPUTPM so the point-mesh metadata
includes the simulation time.

In `@src/pre_process/m_start_up.fpp`:
- Around line 358-361: The Windows implementation of s_create_directory does not
create parent directories recursively, causing failures when code calls
s_delete_directory(...) then s_create_directory(trim(proc_rank_dir)//'/0') (and
similar calls elsewhere); update the Windows branch in the s_create_directory
implementation (in m_compile_specific) to invoke mkdir with the /s flag (e.g.,
use "mkdir /s <path>" or the equivalent command string) so parent directories
are created recursively, and ensure any other uses of s_create_directory (the
second occurrence noted) benefit from the same change.

[codecov]

Codecov Report

❌ Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@598f5a5). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/post_process/m_data_output.fpp	40.00%	3 Missing ⚠️
src/pre_process/m_simplex_noise.fpp	0.00%	2 Missing ⚠️
src/pre_process/m_start_up.fpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1240   +/-   ##
=========================================
  Coverage          ?   45.36%           
=========================================
  Files             ?       70           
  Lines             ?    20515           
  Branches          ?     1954           
=========================================
  Hits              ?     9306           
  Misses            ?    10082           
  Partials          ?     1127           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wilfonba]

Looks good

[github-actions]

Claude Code Review

Head SHA: bf82a80
Files changed: 5

• src/common/m_model.fpp
• src/post_process/m_data_output.fpp
• src/pre_process/m_simplex_noise.fpp
• src/pre_process/m_start_up.fpp
• src/simulation/m_global_parameters.fpp

---

Summary

• Six targeted bug fixes across pre-process, post-process, and common code; none touch GPU kernels or MPI hot paths.
• All six root-cause analyses in the PR description are accurate and the diffs implement the stated fixes correctly.
• The only meaningful behavioural change beyond the six described fixes is the cycle → exit substitution in the interface-data loop, which is the correct fix for the stale-index bug.
• One inaccuracy in PR metadata: src/simulation/m_global_parameters.fpp is modified, contradicting the test-plan claim "No Fortran source files in simulation hot path are touched."
• src/common/m_model.fpp affects all three executables (acknowledged in the PR description).

---

Findings

src/simulation/m_global_parameters.fpp (line ~818) — PR metadata inaccuracy
The fix (ymin/ymax → zmin/zmax) is safe and confined to the default-value initialisation loop in s_initialize_global_parameters. However, the test-plan bullet "No Fortran source files in simulation hot path are touched" is incorrect; this file is in src/simulation/. The fix itself is low-risk, but CI should confirm it.

src/post_process/m_data_output.fpp (loop at ~1548) — cycle → exit semantics
The behavioural change is intentional and correct. With cycle, finding a close point (euc_d < tgp) for iteration i would skip that iteration and keep looping; the new point could still be added on the i == counter branch in a later iteration regardless. With exit, finding any close point immediately aborts the loop and prevents the add — which is the correct semantics ("skip this candidate if it is already covered by any existing point"). Confirmed correct.

src/post_process/m_data_output.fpp (lines ~1102, ~1272) — time_real assignment placement
time_real = file_time is placed after the MPI broadcast of file_time, so all ranks have a consistent value before it is used. Correct.

src/pre_process/m_start_up.fpp (~line 507) — s_read_serial_ic_data_files path
Original code called s_create_directory(trim(proc_rank_dir)//'/*'), which would create a literal directory named *. The fix calls s_delete_directory then s_create_directory(…//'/0'). One thing to verify: s_delete_directory now removes the entire proc_rank_dir tree, so the immediately following s_create_directory call must tolerate a missing parent. If s_create_directory uses mkdir -p (or equivalent) this is fine; worth confirming in s_create_directory's implementation, but the existing call at line ~358 follows the same pattern so it is almost certainly handled.

src/pre_process/m_simplex_noise.fpp (lines ~208-209) — mod(i,255) → iand(i,255)
mod(i,255) produces values 0..254 (skipping 255), while iand(i,255) produces 0..255. Fix is correct for a 256-entry permutation table indexed 0..255.

src/common/m_model.fpp (lines ~252, ~300-306) — OBJ reader iv3
j was overwritten by read(line(3:), *) k, l, j before being used as both the triangle counter (model%trs(j)) and the third vertex index (vertices(j,:)). Introducing iv3 cleanly separates the two roles. Fix is correct.

---

Minor note

No forbidden patterns (dsqrt, 1.0d0, raw !$acc, etc.) introduced. Precision types and allocation/deallocation balance are unaffected.

[github-actions]

Claude Code Review

Head SHA: bf82a80

Files changed: 5

• src/common/m_model.fpp
• src/post_process/m_data_output.fpp
• src/pre_process/m_simplex_noise.fpp
• src/pre_process/m_start_up.fpp
• src/simulation/m_global_parameters.fpp

---

Summary

• Batch of 6 targeted bug fixes in pre/post-process and common CPU paths; no GPU kernel or MPI hot-path code touched.
• Each fix is small (1–4 lines) and addresses a clearly identified defect (uninit vars, wrong indices, wrong function call, wrong operator).
• Changes to src/common/m_model.fpp affect all three executables (pre_process, simulation, post_process); the OBJ reader is CPU-only and the fix is safe.
• No new parameters, no precision-system changes, no GPU macro usage needed.

---

Findings

1. OBJ reader — correct fix, but j array bounds need scrutiny src/common/m_model.fpp ~L297

The fix correctly introduces iv3 to prevent j (triangle counter) from being overwritten by the third vertex index parsed from the face line. The repair logic is sound:

read (line(3:), *) k, l, iv3
model%trs(j)%v(1, :) = vertices(k, :)
model%trs(j)%v(2, :) = vertices(l, :)
model%trs(j)%v(3, :) = vertices(iv3, :)
j = j + 1

No issues here. The typo fix (odj → obj) in the doc comment is a minor bonus.

2. time_real initialisation — correct src/post_process/m_data_output.fpp ~L1102 & ~L1272

time_real = file_time is inserted after the MPI_BCAST of file_time, ensuring all ranks see the correct value before writing. Both call sites (Lagrangian bubble I/O subroutines) are fixed identically. Looks good.

3. s_write_intf_data_file proximity loop — correct but logic shift is non-trivial src/post_process/m_data_output.fpp ~L1548

The old code computed euc_d before the inner loop using the outer loop variable i (stale/wrong point), and used cycle where exit was needed. The new code moves euc_d inside the loop (correct per-point comparison) and replaces cycle with exit (stop searching once a nearby point is found). The redundant euc_d > tgp guard in the elseif is also correctly dropped since the exit on euc_d < tgp already ensures the elseif is only reached when euc_d >= tgp.

One thing to confirm: the outer if branch (counter increment path) previously also computed euc_d and tgp after incrementing, then immediately fell through — those values were dead code since the else branch is mutually exclusive. Removing them is correct.

4. Simplex noise — iand(i, 255) is the right operator src/pre_process/m_simplex_noise.fpp ~L208

mod(i, 255) yields 0–254, skipping index 255 of the 256-entry permutation table (p_vec(0:255)). iand(i, 255) is equivalent to mod(i, 256) for non-negative inputs and also works correctly for negative integer inputs (bitwise AND always returns 0–255 in two's complement), making it both correct and more robust than mod(i, 256).

5. Directory management — correct src/pre_process/m_start_up.fpp ~L357 & ~L507

Fix 1 (L357): Was passing path/* to s_delete_directory, which (depending on implementation) may have created a literal * entry. Now passes path directly.

Fix 2 (L507): Was calling s_create_directory(path/*) — wrong function and wrong argument. Now correctly calls s_delete_directory(path) before the subsequent s_create_directory(path/0). Since s_create_directory uses mkdir -p semantics, recreating the parent with /0 appended will work without an extra explicit parent-creation step.

6. Integral z-bounds — correct src/simulation/m_global_parameters.fpp ~L818

Copy-paste duplicate ymin/ymax fixed to zmin/zmax. No other concerns.

---

Improvement Opportunities (non-blocking)

1. OBJ reader robustness: The face parser silently ignores lines that aren't "v " or "f ". OBJ files can legally contain normals (vn), texcoords (vt), groups (g), etc. The existing case default: print *, "Error: ..." path is fine as a warning but does not abort; if a quad face (f a b c d) is encountered, the extra index is silently dropped. This predates this PR and is outside scope — just worth noting for future robustness work.

2. s_write_intf_data_file upper bound on x_d1/y_d1: counter is incremented unboundedly inside the loop. If the arrays were allocated to a fixed size, this could overflow. Worth verifying that the allocation size is an upper bound on the number of interface points that can be selected.

3. Test coverage: The PR description mentions no dedicated test case for the interface-data-file or OBJ-reader paths. If golden-file tests exist for these features, regeneration confirmation would strengthen confidence.

---

Overall: All six fixes are correct and the changes are minimal and focused. The src/common/ change is safe (CPU-only OBJ parsing). No precision, GPU, or MPI concerns. Recommend merging once CI passes.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request contains five distinct corrections and adjustments across multiple preprocessing, simulation, and postprocessing modules. Changes include fixing OBJ file parsing to properly track vertex indices using a dedicated variable, correcting parameter documentation, refactoring volume-fraction interface extraction logic with modified euc_d and tgp computations, optimizing 2D simplex noise indexing through bitwise AND operations, changing directory cleanup granularity from partial to full removal during preprocessing, and fixing default bounds initialization to use z-dimension parameters instead of repeated y-dimension assignments in global parameter setup.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the batch nature of 6 low-risk bug fixes across pre/post-process modules, clearly summarizing the main change.
Description check	✅ Passed	The description comprehensively covers all six fixes with linked issue numbers, test plan items, and clear impact statements, matching the repository template structure.
Linked Issues check	✅ Passed	All six implemented fixes (#1214, #1177, #1178, #1189, #1191, #1218) directly address the primary coding requirements and objectives stated in their linked issues.
Out of Scope Changes check	✅ Passed	All changes are scoped to the six identified bugs: directory cleanup, integral z-bounds, OBJ reader indexing, time initialization, interface distance computation, and simplex noise wrapping.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.

OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/post_process/m_data_output.fpp (1)

1271-1275: ⚠️ Potential issue | 🟠 Major

This still doesn't affect the formatted-output timestamp.

time_real is never consumed after Line 1275 in s_write_lag_bubbles_to_formatted_database_file, so this branch still never propagates file_time into any database write.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 473031e5-684c-4dae-8a36-27629f64ec4c

📥 Commits

Reviewing files that changed from the base of the PR and between 598f5a5 and bf82a80.

📒 Files selected for processing (5)

• src/common/m_model.fpp
• src/post_process/m_data_output.fpp
• src/pre_process/m_simplex_noise.fpp
• src/pre_process/m_start_up.fpp
• src/simulation/m_global_parameters.fpp

[github-actions]

Claude Code Review

Head SHA: aff8877

Files changed: 5

• src/common/m_model.fpp
• src/post_process/m_data_output.fpp
• src/pre_process/m_simplex_noise.fpp
• src/pre_process/m_start_up.fpp
• src/simulation/m_global_parameters.fpp

Summary

• Batch of 6 isolated bug fixes, all in pre/post-process or CPU-only paths; no simulation hot path or GPU code is touched.
• Each fix is small (1–4 lines) and clearly addresses the stated bug. Logic in all cases checks out.
• The directory-management fix and the interface-loop restructuring are the most behaviorally significant changes and deserve the closest scrutiny.

---

Findings

1. m_start_up.fpp — Directory deletion may leave parent absent (verify s_create_directory semantics)

Both hunks now call s_delete_directory(trim(proc_rank_dir)) (deletes the directory itself), followed immediately by s_create_directory(trim(proc_rank_dir)//'/0'). After a full deletion, proc_rank_dir no longer exists, so s_create_directory must internally use mkdir -p (or equivalent) to recreate the parent before creating the /0 subdirectory.

The old wildcard pattern (proc_rank_dir/\*) was wrong, but it did leave proc_rank_dir in place, so creating the /0 child was safe. Please confirm that s_create_directory handles missing parents, or add an explicit s_create_directory(trim(proc_rank_dir)) call before creating /0. If CI creates and tears down per-rank directories, this may not be caught until a run with an already-absent parent directory.

2. m_data_output.fpp — Interface-point loop logic restructuring (minor, correct but subtle)

! New:
do i = 1, counter
    euc_d = sqrt((x_cc(j) - x_d1(i))**2 + (y_cc(k) - y_d1(i))**2)
    if (euc_d < tgp) then
        exit                    ! reject new point — too close to an existing one
    elseif (i == counter) then
        counter = counter + 1   ! no nearby point found — accept
        x_d1(counter) = x_cc(j)
        y_d1(counter) = y_cc(k)
    end if
end do

The semantics are now: reject the new candidate point as soon as any existing interface point is within tgp; accept only if no such neighbour is found after checking all counter points. This is correct. The old code computed euc_d outside the inner i loop using the outer loop variable j/k (so x_d1(i) was never varied over stored points), making the comparison meaningless. The removed elseif (euc_d > tgp .and. i == counter) redundant guard is safe to drop because exit already handles the euc_d < tgp case.

No issue — flagging only for reviewer awareness of the behavioral change.

3. m_simplex_noise.fpp — mod(i, 255) → iand(i, 255) (+1)

mod(i, 255) produces values 0–254 (never 255), silently skipping the last permutation-table entry. iand(i, 255) (bitmask 0xFF) produces 0–255 as the algorithm intends. Fix is correct.

One note: this implies p_vec must be validly addressable at index 255. Since the permutation table in simplex noise is conventionally 256 entries (0-based), verify p_vec is declared with a lower bound of 0 (or at least 256+ elements with Fortran's default 1-based indexing) so p_vec(255) doesn't become an out-of-bounds access. If p_vec is 1-indexed with 256 elements, index 255 is in-bounds — fine. Just worth a quick check.

4. m_model.fpp — OBJ index fix (straightforward, correct)

Introducing iv3 cleanly separates the triangle counter j from the third vertex index. The original read(...) k, l, j clobbered j before using it as the triangle index in model%trs(j)%v. Fix is correct.

5. m_global_parameters.fpp — zmin/zmax copy-paste fix (trivially correct)

Duplicate ymin/ymax assignments replaced with zmin/zmax. No issues.

6. m_data_output.fpp — time_real = file_time (correct)

Added in two symmetric call sites after the MPI_BCAST for file_time. Initialises time_real from the broadcast value before it is used in output. Correct.

---

Overall: All 6 fixes are logically sound. The one item worth a second look before merge is the s_create_directory parent-directory assumption in m_start_up.fpp (finding #1).