Skip to content

Clean ib patch file#1413

Merged
sbryngelson merged 4 commits intoMFlowCode:masterfrom
danieljvickers:clean-ib-patch-file
May 9, 2026
Merged

Clean ib patch file#1413
sbryngelson merged 4 commits intoMFlowCode:masterfrom
danieljvickers:clean-ib-patch-file

Conversation

@danieljvickers
Copy link
Copy Markdown
Member

@danieljvickers danieljvickers commented May 8, 2026

Description

Deletes the old cartesian coordinate variables from IB patches. We never have supported cartesian coordinates for patches, and thus this code is unneccessary. It is now causing issues on reldebug builds, and thus should go.

Fixes #(issue)

Type of change

  • Bug fix
  • Refactor

@qodo-code-review
Copy link
Copy Markdown
Contributor

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Claude Code Review

Head SHA: 26244e5

Files changed:

  • 1
  • src/simulation/m_ib_patches.fpp

Findings:

1. Correctness regression: sphere IB patch broken on cylindrical grids (grid_geometry == 3)

The removed hunk in s_ib_sphere previously converted cylindrical coordinates to Cartesian before the containment check:

! removed:
if (grid_geometry == 3) then
    call s_convert_cylindrical_to_cartesian_coord(y_cc(j), z_cc(k))
    ! sets cart_y = r*sin(theta), cart_z = r*cos(theta)
else
    cart_y = y_cc(j)
    cart_z = z_cc(k)
end if
if (... (cart_y - center(2))**2 + (cart_z - center(3))**2 <= radius**2) ...

The replacement uses y_cc(j) and z_cc(k) directly:

if (((x_cc(i) - center(1))**2 + (y_cc(j) - center(2))**2 + (z_cc(k) - center(3))**2 <= radius**2)) then

On a cylindrical grid, y_cc(j) holds the radial coordinate r and z_cc(k) holds the azimuthal angle theta (radians). The distance check now computes (r - y_centroid)^2 + (theta - z_centroid)^2, mixing length and angle units. This produces geometrically incorrect sphere membership results for any 3D IB case that uses a cylindrical-coordinate grid. The cuboid and cylinder patches had an existing TODO noting they were already broken for cylindrical grids; the sphere did not have that caveat and was previously handled correctly via the coordinate conversion that this PR removes.


2. Removed public symbol f_convert_cyl_to_cart may break callers

The diff removes f_convert_cyl_to_cart from both the module public list and the module body. This was a pure, GPU-device-callable function ($:GPU_ROUTINE(parallelism='[seq]')) that was explicitly exported. Any use m_ib_patches site elsewhere in src/simulation/ that references f_convert_cyl_to_cart will fail to compile. The function's prior presence in the public API makes it plausible it has callers outside this file.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.92%. Comparing base (141ff01) to head (1942141).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1413      +/-   ##
==========================================
+ Coverage   64.90%   64.92%   +0.02%     
==========================================
  Files          72       72              
  Lines       18874    18855      -19     
  Branches     1571     1570       -1     
==========================================
- Hits        12250    12242       -8     
+ Misses       5649     5639      -10     
+ Partials      975      974       -1     

☔ 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 merged commit 35cebdf into MFlowCode:master May 9, 2026
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants