Skip to content

mpi: Honor MultiSubDomain thicknesses in overlap modes - #3075

Draft
ggorman wants to merge 1 commit into
mainfrom
fix-multisubdomain-overlap
Draft

ggorman wants to merge 1 commit into
mainfrom
fix-multisubdomain-overlap

Conversation

@ggorman

@ggorman ggorman commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Under DEVITO_MPI=overlap, overlap2 and full, an equation over a SubDomainSet skipped points near the box edges: the halo scheme only honored SubDimension thicknesses, so the box thickness was applied relative to each CORE/OWNED region instead of being intersected with it. basic and diag2 were fine.

  • HaloScheme now honors MultiSubDimension thicknesses too, and records them as runtime thicknesses (they are set inside the Operator, one subdomain at a time). This alone fixes overlap.
  • overlap2/full fill the OWNED region struct at argument time, when those values don't exist yet, so the remainder applies them relative to the region bounds instead.

Found through DevitoPRO's ABox (devitocodespro/devitopro#941), which gave wrong FWI gradients under full. New test: test_subdomainset_stencil_mpi fails under overlap/overlap2/full on main and passes for all five modes here. Generated code for regular SubDomains is unchanged.

🤖 Generated with Claude Code

Under DEVITO_MPI=overlap/overlap2/full the CORE and OWNED regions ignored
the thicknesses of MultiSubDimensions, so points near the edges of a
SubDomainSet (or DevitoPRO ABox) were skipped. These thicknesses are only
known at runtime, so overlap2/full apply them to the OWNED region bounds in
the remainder rather than storing them in the region struct.
@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 31.57895% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.06%. Comparing base (8bbe0d3) to head (0a1835a).

Files with missing lines Patch % Lines
tests/test_subdomains.py 8.33% 22 Missing ⚠️
devito/mpi/routines.py 5.55% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3075      +/-   ##
==========================================
- Coverage   84.23%   84.06%   -0.17%     
==========================================
  Files         258      258              
  Lines       57074    57126      +52     
  Branches     4875     4882       +7     
==========================================
- Hits        48076    48024      -52     
- Misses       8170     8271     +101     
- Partials      828      831       +3     
Flag Coverage Δ
pytest-gpu-aomp-amdgpuX 68.47% <42.42%> (-0.04%) ⬇️
pytest-gpu-gcc- 78.96% <31.57%> (-0.05%) ⬇️
pytest-gpu-icx- 78.86% <31.57%> (-0.07%) ⬇️
pytest-gpu-nvc-nvidiaX 69.04% <42.42%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

@ggorman
ggorman marked this pull request as ready for review October 19, 2026 19:02

@mloubout mloubout left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't look right, having to track MultiSubs seaprately looks overly complicated, and having a haloscheme having separate arguments and runtime_tkns is not right.

@ggorman

ggorman commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

The osx failure is the Apple-silicon detection flake, not this change: the job header reads platform: cpu64 on the arm64 runner, so the cpu64-arm skip does not apply and test_nested_nthreads fails compiling x86 intrinsic headers (https://github.com/devitocodes/devito/actions/runs/39303223750/job/118233531439). #3048 fixes the detection. The other 41 jobs pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants