Skip to content

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

Draft
sbryngelson wants to merge 12 commits intoMFlowCode:masterfrom
sbryngelson:fix/safe-bugfixes-batch
Draft

Fix 6 safe pre/post-process bugs (batch)#1240
sbryngelson wants to merge 12 commits intoMFlowCode:masterfrom
sbryngelson:fix/safe-bugfixes-batch

Conversation

@sbryngelson
Copy link
Member

@sbryngelson sbryngelson commented Feb 22, 2026

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

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

Summary by CodeRabbit

  • Bug Fixes

    • Corrected vertex index handling in OBJ file reader for accurate face parsing.
    • Fixed time value propagation in bubble-related I/O operations.
    • Improved Euclidean distance computation logic in interface data output.
    • Optimized simplex noise index wrapping for better performance.
  • Refactor

    • Adjusted default probe coordinate bounds initialization from y-dimension to z-dimension.

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.

Copilot AI review requested due to automatic review settings February 22, 2026 20:54
@codeant-ai codeant-ai bot added the size:S This PR changes 10-29 lines, ignoring generated files label Feb 22, 2026
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 22, 2026

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.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

Confidence score: 5/5

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

codecov bot commented Feb 23, 2026

Codecov Report

❌ Patch coverage is 30.76923% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.06%. Comparing base (df28255) to head (242d474).
⚠️ Report is 29 commits behind head on master.

Files with missing lines Patch % Lines
src/post_process/m_data_output.fpp 40.00% 3 Missing ⚠️
src/common/m_model.fpp 0.00% 2 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   44.05%   44.06%   +0.01%     
==========================================
  Files          70       70              
  Lines       20496    20497       +1     
  Branches     1989     1989              
==========================================
+ Hits         9030     9033       +3     
+ Misses      10328    10326       -2     
  Partials     1138     1138              

☔ 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.

@sbryngelson sbryngelson force-pushed the fix/safe-bugfixes-batch branch from 0dabbbc to 08a7f25 Compare February 23, 2026 14:48
@codeant-ai codeant-ai bot added size:S This PR changes 10-29 lines, ignoring generated files and removed size:S This PR changes 10-29 lines, ignoring generated files labels Feb 23, 2026
@MFlowCode MFlowCode deleted a comment from github-actions bot Feb 23, 2026
@sbryngelson sbryngelson force-pushed the fix/safe-bugfixes-batch branch 2 times, most recently from 63ae8fe to dd568ab Compare February 24, 2026 16:03
@codeant-ai codeant-ai bot added size:S This PR changes 10-29 lines, ignoring generated files and removed size:S This PR changes 10-29 lines, ignoring generated files labels Feb 24, 2026
sbryngelson and others added 6 commits February 24, 2026 11:50
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…a_files

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The z-bounds initialization for volume integrals repeats ymin/ymax
instead of zmin/zmax. 3D volume integrals use uninitialized z-bounds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When reading face lines, j is overwritten by the third vertex index
from the file, then used as the triangle index for model%trs(j).
Introduces a separate iv3 variable for the third vertex index.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
time_real is declared but never assigned from file_time after the
MPI broadcast. The time column in output contains garbage.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
euc_d was computed outside the inner loop using the outer variable i
(stale from a previous loop) instead of the current stored-point
index. Moved distance computation inside the do-loop over stored
points and changed cycle to exit for correct early termination when
a candidate point is too close to any existing point.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sbryngelson sbryngelson force-pushed the fix/safe-bugfixes-batch branch from 770e05a to bb2d57a Compare February 24, 2026 16:50
@codeant-ai codeant-ai bot added size:S This PR changes 10-29 lines, ignoring generated files and removed size:S This PR changes 10-29 lines, ignoring generated files labels Feb 25, 2026
Copy link
Contributor

@wilfonba wilfonba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@sbryngelson sbryngelson marked this pull request as draft February 26, 2026 00:09
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from coderabbitai bot Feb 26, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link

Claude Code Review

Head SHA: 242d47449b68fee8a655f72948d914c3e4764415

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 genuine bug fixes, all small and surgical; logic in every hunk is correct.
  • Two fixes touch code outside the stated scope ("pre/post-process only"): src/common/m_model.fpp is shared by all three executables, and src/simulation/m_global_parameters.fpp is simulation code.
  • The cycleexit change in the interface-data loop and the iand simplex-noise fix are the subtlest changes; both are correct.
  • No precision, GPU, MPI, or parameter-system violations detected.

Findings

1. src/common/m_model.fpp — scope mismatch with PR description (informational)

The PR description says all fixes are "pre-process or post-process CPU code paths", but m_model.fpp lives in src/common/ and is compiled into all three executables. Per CLAUDE.md: "Changes to src/common/ affect ALL three executables. Test comprehensively." The fix itself (introducing iv3 to decouple the triangle counter j from the third vertex index read off the f line) is unambiguously correct; the only gap is that CI should exercise all three targets for this file.

2. src/simulation/m_global_parameters.fpp — scope mismatch (informational)

integral(i)%zmin / integral(i)%zmax initialization fix lives in src/simulation/, not pre/post-process. The change is purely in the s_initialize_* startup routine (CPU path, no GPU kernels), so runtime risk is low, but the PR description is inaccurate.

3. src/post_process/m_data_output.fpp lines ~1558–1563 — cycleexit semantics (verify intent)

exit is correct: as soon as one existing point is within tgp, the candidate is discarded. The old cycle continued iterating after a close match, comparing the same stale pre-loop euc_d against every subsequent i; the companion change moving euc_d inside the loop was also necessary. The simplified elseif (i == counter) (dropping the now-redundant euc_d > tgp clause) is also correct because we only reach it when the exit branch was not taken.

4. src/pre_process/m_simplex_noise.fpp lines 208–209 — iand correctness for negative coordinates

mod(i, 255) has two bugs: (a) it never produces 255 (range 0–254), skipping one permutation-table entry; (b) Fortran mod preserves the sign of the dividend, so negative grid-cell indices yield negative values that would be out-of-bounds for p_vec. iand(i, 255) masks to 0–255 and handles negative two's-complement values safely (e.g., iand(-1, 255) = 255). Fix is correct and an improvement over the original in both respects.


No blocking issues found. All six fixes are correct.

@MFlowCode MFlowCode deleted a comment from codeant-ai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from Copilot AI Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from Copilot AI Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from Copilot AI Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from coderabbitai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from coderabbitai bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from github-actions bot Feb 26, 2026
@MFlowCode MFlowCode deleted a comment from github-actions bot Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files

3 participants