Skip to content

Comments

Fix z-boundary backward FD stencil using fields(2) instead of fields(3)#1173

Closed
sbryngelson wants to merge 1 commit intoMFlowCode:masterfrom
sbryngelson:fix/fd-z-boundary-stencil
Closed

Fix z-boundary backward FD stencil using fields(2) instead of fields(3)#1173
sbryngelson wants to merge 1 commit intoMFlowCode:masterfrom
sbryngelson:fix/fd-z-boundary-stencil

Conversation

@sbryngelson
Copy link
Member

@sbryngelson sbryngelson commented Feb 21, 2026

Summary

Severity: CRITICAL — produces wrong results in all 3D simulations using this routine.

File: src/common/m_finite_differences.fpp, line 53

The z-direction upper-boundary backward finite difference stencil at iz_s%end uses fields(2) (the y-component) instead of fields(3) (the z-component) in the third term of the second-order backward difference.

Before

divergence = divergence + (+3._wp*fields(3)%sf(x,y,z) &
    - 4._wp*fields(3)%sf(x,y,z-1) &
    + fields(2)%sf(x,y,z-2)) / (z_cc(z) - z_cc(z-2))
!   ^^^^^^^^^ should be fields(3)

After

divergence = divergence + (+3._wp*fields(3)%sf(x,y,z) &
    - 4._wp*fields(3)%sf(x,y,z-1) &
    + fields(3)%sf(x,y,z-2)) / (z_cc(z) - z_cc(z-2))

Why this went undetected

This is a copy-paste error in the x→y→z stencil chain where only the third term of the z-direction backward stencil is wrong — the first two terms correctly use fields(3). The bug only activates at the exact upper z-boundary cells (iz_s%end) where the one-sided backward stencil is used; the interior central-difference stencil and the lower-boundary forward stencil are both correct. This makes the error localized to a thin boundary layer that is easily masked by the correct values everywhere else.

Test plan

  • Run 3D test case with non-trivial z-boundary divergence
  • Verify divergence values at iz_s%end match the y-boundary analog

🤖 Generated with Claude Code

Fixes #1194

Copilot AI review requested due to automatic review settings February 21, 2026 03:22
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 21, 2026

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 21, 2026

📝 Walkthrough

Walkthrough

The s_compute_fd_divergence routine in the finite differences module was corrected to reference the proper field when computing the z-direction boundary divergence contribution. The affected term now accesses the third field instead of the second field.

Changes

Cohort / File(s) Summary
Finite Differences Boundary Fix
src/common/m_finite_differences.fpp
Corrected field index reference in z-direction boundary divergence computation from fields(2) to fields(3) for improved accuracy in boundary derivative calculations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Suggested labels

Review effort 2/5, size:XS

Poem

🐰 A whisker-twitch away from error
Field three now leads where two did stray
Boundaries fixed with finite precision
Divergence flows the proper way
Mathematics hops back on track!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the bug fix: correcting a field index in the z-boundary finite difference stencil from fields(2) to fields(3).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description is comprehensive and well-structured, addressing severity, motivation, before/after code comparison, root cause analysis, and test plan.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@codeant-ai codeant-ai bot added the size:XS This PR changes 0-9 lines, ignoring generated files label Feb 21, 2026
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 21, 2026

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Boundary-index safety
    The backward stencil at the upper z-boundary accesses z-1 and z-2. Ensure these indices are always valid for every call (ghost/halo cells or domain sizing). If the field arrays or z_cc do not include sufficient padding, this will read out-of-bounds or fetch invalid data.

  • Consistency check
    Confirm there are no other places in the codebase that still use the y-component (fields(2)) for z-derivative boundary terms. The PR fixes one instance; any remaining occurrences will continue to corrupt divergence at the z-boundary.

  • Performance / memory access
    The stencil reads multiple nearby entries from fields(3)%sf and z_cc repeatedly inside a tight GPU-parallel loop. This can harm memory coalescing and increase global memory traffic; caching neighbors into temporaries could reduce loads and improve throughput.

@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 21, 2026

CodeAnt AI finished reviewing your PR.

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

Fixes an incorrect component reference in the z-direction upper-boundary backward finite-difference stencil inside the shared finite-differences module, preventing corrupted divergence results at the z-boundary for 3D runs.

Changes:

  • Correct z-upper boundary backward-difference term to use fields(3) (z-component) rather than fields(2) (y-component).

cubic-dev-ai[bot]

This comment was marked as off-topic.

The z-direction upper-boundary backward difference at iz_s%end uses
fields(2) (y-component) instead of fields(3) (z-component) in the
third term, corrupting the divergence in all 3D simulations using
this finite difference routine.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Feb 21, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 44.05%. Comparing base (84c46e0) to head (dc4ebd7).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/common/m_finite_differences.fpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1173   +/-   ##
=======================================
  Coverage   44.05%   44.05%           
=======================================
  Files          70       70           
  Lines       20498    20498           
  Branches     1990     1990           
=======================================
  Hits         9030     9030           
  Misses      10329    10329           
  Partials     1139     1139           

☔ 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
Copy link
Member Author

Superseded by #1242 (batched HPC-sensitive fixes)

@sbryngelson sbryngelson deleted the fix/fd-z-boundary-stencil branch February 22, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files

Development

Successfully merging this pull request may close these issues.

z-boundary backward FD stencil uses fields(2) instead of fields(3)

1 participant