Skip to content

Add mesh-files-only internal spherical shell generation - #592

Merged
lmoresi merged 1 commit into
underworldcode:developmentfrom
gthyagi:feature/spherical-shell-mesh-files-only
Aug 18, 2026
Merged

Add mesh-files-only internal spherical shell generation#592
lmoresi merged 1 commit into
underworldcode:developmentfrom
gthyagi:feature/spherical-shell-mesh-files-only

Conversation

@gthyagi

@gthyagi gthyagi commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • extract the existing atomic Gmsh-to-PETSc-HDF5 conversion into _gmsh_to_h5();
  • add SphericalShellInternalBoundary(write_mesh_files_only=True);
  • return the generated .msh.h5 path without constructing an in-memory UW3 Mesh;
  • preserve the existing full-Mesh behavior by default;
  • add focused regression coverage for bypassing Mesh construction and reloading the generated HDF5 with Lower, Internal, and Upper labels.

Motivation

SphericalShellInternalBoundary() currently writes the reusable .msh.h5 file during Gmsh import, then reloads that file into a full UW3 mesh and materializes Inner and Outer cell-region labels. The region creation loops over every cell in Python.

That work is required when the caller needs the returned in-memory mesh, but it is unnecessary for workflows that only generate reusable mesh files for later MPI jobs. The custom region labels are created after the HDF5 write and therefore do not alter the already-written reusable file.

This became significant for the Zhong et al. spherical-shell benchmark on Gadi. Each cellSize=1/64 mesh contains about 4.2 million cells. The approximately 710 MB HDF5 files appeared several minutes before the serial jobs exited; one measured job completed in 15:14 using 8.35 GB while remaining CPU-bound in post-conversion mesh setup.

API

h5_path = uw.meshing.SphericalShellInternalBoundary(
    radiusOuter=1.0,
    radiusInternal=0.775,
    radiusInner=0.55,
    cellSize=1.0 / 64.0,
    filename="shell.msh",
    write_mesh_files_only=True,
)

With write_mesh_files_only=True, the generator:

  1. writes the Gmsh .msh file;
  2. atomically converts it to PETSc .msh.h5;
  3. preserves the imported physical boundary labels;
  4. returns the HDF5 path as a string;
  5. skips HDF5 reload, coordinate-system initialization, and cell-region classification.

The default remains write_mesh_files_only=False, so existing callers continue to receive a fully initialized UW3 Mesh with no behavior change.

Validation

Validated on a fresh branch from upstream/development at 59982834:

  • ./uw build completed successfully;
  • file-generation regression passed;
  • unchanged full-constructor boundary-area regression passed;
  • focused result: 2 passed;
  • Python syntax and whitespace checks passed.

The same feature diff also passed the complete level-1 suite before being squashed for this PR: 1493 passed, 33 skipped, 2 xfailed.

Extract the atomic Gmsh-to-PETSc-HDF5 conversion from _from_gmsh so SphericalShellInternalBoundary can write reusable mesh files without loading and initializing an Underworld Mesh. Add write_mesh_files_only=True to return the .msh.h5 path after conversion while preserving the existing full-Mesh behavior by default. This avoids the serial Inner/Outer classification over millions of cells when callers only need reusable mesh files. Add a level-2 regression that verifies Mesh construction is bypassed and the HDF5 reloads with Lower, Internal, and Upper labels.
@lmoresi

lmoresi commented Aug 18, 2026

Copy link
Copy Markdown
Member

Adversarial review — spherical-shell cluster (#591, #592, #593, #594)

Reviewed as a group because they overlap: #594 contains #591, and all four land
in the spherical/postprocessing path. Findings below are labelled by the PR they
apply to; the cluster-level ones come first because a single-PR reading misses
them.

Cluster

C1. #594 is stacked on #591 — merging #594 merges #591. pr591 is an
ancestor of pr594, and postprocessing/geoid.py and
postprocessing/__init__.py are byte-identical between them (git diff pr591 pr594 -- ... is empty). #594 is #591 plus two commits (95b3470, fb9ce95).
Neither PR body says so. Merge #591 first and #594 becomes a two-commit
follow-up; merge #594 first and #591 closes as already-merged. What must not
happen is reviewing the geoid code twice as though two implementations were on
offer.

C2. tests/test_1029_ is used twice after the merge. #550 landed
tests/test_1029_analytic_faulted_medium.py yesterday; #594 adds
tests/test_1029_analytic_zhong2008.py. Both survive the merge — we resolved
the tree and they are both there. Two different subjects under one number
defeats the numbering. Zhong should move to 1030.

C3. Not a defect, recorded because the diff view says otherwise. GitHub
shows #594 REMOVING FaultedMedium from analytic/__init__.py — the import,
the __all__ entry and the _SOLUTIONS entry. That is the diff against the
merge base, not against development. We resolved the merge (git merge-tree --write-tree origin/development pr594): it is clean, and the resulting
__init__.py carries FaultedMedium and Zhong2008 side by side, with
barr_houseman_96.py intact. No rebase is needed on that account.

#594 — Zhong 2008

1. Zhong2008 and Zhong2008Response are in __all__ but not in
_SOLUTIONS.
So uw.analytic.available() does not list them, describe()
does not know them, and neither the conformance suite nor
convention_audit.py measures them. That follows from what they are — neither
subclasses AnalyticSolution, and there is no sample_points, boundaries,
apply_boundary_conditions or set_fields — so the registry would reject them
anyway. It is the right call and it is invisible: everything else reachable as
uw.analytic.X is a solution carrying the validation contract, and these two are
not. Worth a sentence in the module docstring saying they are a response
calculator rather than a solution, so the next reader does not go looking for the
momentum-residual row.

#593 — boundary-flux recovery

2. The memory bound moves, it does not fall. Replacing comm.allgather with
gather-to-rank-zero removes the ×nranks replication, which is what #408 asks
for. Rank zero still holds the entire global boundary and solves the entire
sparse system alone, so the worst-case single-rank memory is unchanged and the
solve is now serial where it used to be redundantly parallel. On the mesh sizes
that motivated #408 that is a clear win; at the scale where rank zero itself is
the constraint it is not, and the docstring's "avoids replicating the global P2
surface mesh and sparse solve on every rank" is true without saying which of the
two limits now binds.

3. Every rank must reach the gather. The recovery is collective by
construction now. A rank owning no part of the boundary has nothing to
contribute, and it still has to enter gather and the scatter that follows it.
We have not run this at a rank count that starves a rank of boundary facets, and
the tests in the PR do not construct one. This is the same shape as the defect
fixed in #596, and it is worth a parallel test with a boundary that does not
reach every rank.

#592 — internal spherical shell

4. It edits test_0502_boundary_integrals.py, which is on the collection
ratchet.
That file does module-level work at import and is exempted by name in
tests/conftest.py (#587). Editing it is fine; adding more module-level work to
it is not, because the exemption hides it. Worth checking the added lines land
inside test functions.

5. discretisation_mesh.py is touched by #596 as well. The regions are far
apart — #592 near line 100, #596 near 8500 — so they merge textually. No action;
recorded so the collision is not discovered at merge time.

What we did not do

None of the four was run. The findings above come from reading the diffs and
resolving the merges mechanically; C1, C2 and C3 are verified against the git
objects, and 1 to 5 are read from the source. Where a claim needed a measurement
we have said so rather than asserting it.

Underworld development team with AI support from Claude Code

@lmoresi
lmoresi merged commit a6be730 into underworldcode:development Aug 18, 2026
4 checks passed
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