Fix MPI ghost-cell bug in FormFunctionLocal - #69
Open
awickert wants to merge 3 commits into
Open
Conversation
## Ghost-cell bug FormFunctionLocal cached inverse transmissivity in a global DMDA vector (T_vec) and then accessed neighbor indices my_T[j][i±1] / my_T[j±1][i]. Under MPI, DMDAVecGetArray on a global vector covers only the locally owned range, so those neighbor accesses were out-of-bounds at processor boundaries. With a single process the bug was latent; with >=2 processes it produced wrong fluxes at subdomain edges and could SEGV. Fix: replace T_vec (global) with T_local (local ghost vector). Add topo_local, fdepth_local, ksat_local as local ghost vectors for the fields that T depends on. These are populated and scattered via scatter_static_fields() — called once after global vectors are set and before DMDA_Array_Pack acquires its GetArray locks, avoiding a lock conflict. FormFunctionLocal now computes 1/T over the full ghost range [gys..gys+gym) x [gxs..gxs+gxm) so all neighbor lookups are valid. topo_vec, fdepth_vec, ksat_vec are excluded from DMDA_Array_Pack to keep the scatter path free of GetArray locks. ## Anderson as default The original code has no SNESSetType() call, delegating solver selection entirely to runtime -snes_type options. In practice, Anderson mixing is used (as in the benchmark scripts) because it converges reliably without a hand-coded Jacobian. Setting Anderson (m=1) as the compiled-in default makes the binary work correctly out of the box. Runtime -snes_type overrides still take effect via SNESSetFromOptions(). ## CMakeLists.txt Added explicit OpenMPI include/link paths required on this build system. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dation test Three changes: 1. DMDA scatter sequencing (DMDA_array_pack.cpp): The three DMGlobalToLocalBegin calls in scatter_static_fields shared one PetscSF object on the same DM. Interleaving Begin/Begin/Begin/End/End/End corrupted PETSc internal state (PetscSFLinkGetInUse error at runtime). Each Begin/End pair is now completed sequentially. 2. MPI output gathering (transient_groundwater.cpp, WTM.cpp): Each MPI rank previously wrote only its owned cells back to arp.wtd; the rest of the array retained stale values. saveGDAL called from all ranks on the same path was a race condition. After every solve, arp.wtd is now assembled across ranks via MPI_Allreduce(SUM) — each rank contributes its owned cells and zeros elsewhere — so all ranks hold the complete field before only rank 0 writes to disk. 3. Ghost-cell validation test (tests/ghost_cell/): Integration test that runs WTM on a 12x3 equilibrium domain with a factor-of-10 ksat discontinuity at the MPI processor boundary (columns 0-5 vs 6-11). Compares 1-process and 2-process outputs; they must agree to within 1e-4 m. With the ghost- cell bug the two halves are hydrologically decoupled and the solutions diverge by O(10 m); with the fix they are identical to machine precision. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The hardcoded /usr/lib/x86_64-linux-gnu/openmpi paths are specific to one build system and should not be part of the upstream patch. MPI is already pulled in transitively via PkgConfig::PETSC on standard installations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When WTM is run with more than one MPI process,
FormFunctionLocalcomputestransmissivity from global DMDA vectors (
topo_vec,fdepth_vec,ksat_vec)and accesses neighbour indices
my_T[j][i±1]at subdomain edges.DMDAVecGetArrayon a global vector covers only the locally owned range,so those neighbour accesses fall outside valid memory at every processor
boundary. In practice PETSc zero-initialises these vectors, so the out-of-
range reads return zero values for
fdepthandksat. Withfdepth = 0,depthIntegratedTransmissivityreturns zero, the harmonic-mean facetransmissivity at the boundary collapses to zero, and an implicit no-flow wall
is imposed across every MPI subdomain edge.
Each subdomain then converges correctly — to the solution of a physically wrong
problem in which no groundwater crosses processor boundaries. The solver does
not report any error, and results look plausible at a glance because the
imposed walls are invisible in the output and groundwater diffusion is a local
process. However, large-scale flow paths that cross a processor boundary are
suppressed: the water table is anomalously high on the upstream side and
anomalously low on the downstream side of each internal boundary, with the
error decaying with distance from the wall. Single-process runs are unaffected.
Changes
src/CreateSNES.hpp/src/DMDA_array_pack.hpp/src/DMDA_array_pack.cppReplace
T_vec(global) withT_local(local ghost vector). Addtopo_local,fdepth_local,ksat_localas local ghost vectors.A new
scatter_static_fields()function populates the global topo/fdepth/ksatvectors from
ArrayPackand scatters them to the local ghost vectors beforeDMDA_Array_Packacquires itsGetArraylocks — which would otherwise blockthe scatter. Each
DMGlobalToLocalBegin/Endpair is completed sequentially(the DMDA shares one
PetscSFacross all scatter operations, so interleavedcalls corrupt internal state).
src/transient_groundwater.cppFormFunctionLocalnow uses the local ghost vectors and computes1/Toverthe full ghost range so all neighbour lookups are valid under MPI.
After each solve,
arp.wtdis assembled across all ranks viaMPI_Allreduce(SUM)so that every rank holds the complete field before output.src/WTM.cppOutput is written by rank 0 only, avoiding a file race condition when multiple
ranks called
saveGDALon the same path.src/CreateSNES.cppSNESSetType(SNESANDERSON)is set as the compiled-in default. The binarypreviously required
-snes_type andersonto be passed explicitly at runtime(as in the benchmark scripts); without it PETSc defaulted to Newton, which
diverges immediately without a hand-coded Jacobian.
tests/ghost_cell/Integration test: runs a 12×3 equilibrium domain with a factor-of-10
ksatdiscontinuity placed at the MPI processor boundary, then compares 1-process
and 2-process outputs. With the fix they agree to machine precision (~10⁻¹⁴ m).
Without the fix the two halves are hydrologically decoupled and diverge by
O(10 m). Run with
cd tests/ghost_cell && ./run_test.sh.Caveats
The
MPI_Allreduceoutput-gathering approach intransient_groundwater.cppworks correctly when starting from
wtd = 0(each rank's non-owned cells arezero and the sum assembles cleanly). For multi-cycle runs the same logic holds
because non-owned cells are zeroed before each reduction. This should be
reviewed if support for non-zero supplied initial water tables (
supplied_wt)under MPI is needed in future.
🤖 Generated with Claude Code