mesh: add get_mean_radius() + @collective_operation on radii accessors#206
Merged
Conversation
Adds a parallel-safe mean-cell-radius accessor mirroring the existing get_min_radius / get_max_radius pattern (MPI allreduce of local sum and count). Together the three methods form the canonical "characteristic mesh length" API. Motivation: user code (e.g. an adaptive mesh harness) reached for mesh._radii.mean() to set a smoothing length default, which is rank-local: different ranks compute different means → different gradient_smoothing_length passed to a screened-Poisson Vector_Projection → different JIT C source generated per rank → the JIT determinism guard correctly raised a "C-source hash differs across ranks" RuntimeError. Symptom looked like a JIT bug but was a rank-local-data bug being correctly caught. Also decorates all three radii methods with @uw.collective_operation so any future use inside selective_ranks() blocks raises a clear deadlock-warning before the run hangs. The methods are documented as parallel-safe and the get_mean_radius docstring explicitly warns against falling back to self._radii.mean() with the JIT-leak explanation, so the same trap doesn't get rebuilt in user scripts. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a parallel-safe “mean mesh radius” accessor to Mesh, and marks the radius accessors as collective operations so they fail fast when called inside selective_ranks() (preventing MPI deadlocks and avoiding rank-divergent values leaking into JIT inputs).
Changes:
- Added
Mesh.get_mean_radius()implemented via MPI allreduce of local sum and count. - Decorated
get_min_radius(),get_max_radius(), andget_mean_radius()with@uw.collective_operationto detect unsafe selective-rank usage early.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ests Three review comments from copilot-pull-request-reviewer on PR #206 addressed: * get_mean_radius now passes op=MPI.SUM explicitly to allreduce instead of relying on the default (matches the codebase pattern used elsewhere; less brittle if mpi4py wrapper conventions change) * docstring corrected — clarifies that _radii is the characteristic cell length (volume^(1/dim)) computed by DMPlexComputeGeometryFVM, not literally a centroid-to-face distance. The same correction applies to get_min/max_radius but those existing docstrings are not touched in this PR * test coverage added: - tests/test_0008_mesh_radii_accessors.py: serial unit tests on Annulus and UnstructuredSimplexBox checking min/mean/max ordering and that all three methods carry the @uw.collective_operation decorator - tests/parallel/ptest_0008_mesh_radii_accessors.py: MPI test asserting all ranks see identical min/mean/max to 1e-12 Underworld development team with AI support from Claude Code
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
mesh.get_mean_radius()to complete theget_min_radius / get_max_radius / get_mean_radiustriple — parallel-safe via MPI allreduce of local sum and count.@uw.collective_operationso any use insideselective_ranks()raises a clear deadlock-warning before the run hangs.Motivation
User code (an adaptive-mesh harness,
scripts/stagnant_lid_adapt_loop.pyon a feature branch) reached formesh._radii.mean()to set a smoothing-length default. That's rank-local: each MPI rank computes its own mean → differentgradient_smoothing_lengthpassed into a screened-PoissonVector_Projection→ JIT generates different C source on different ranks → the existing JIT determinism guard correctly raisedRuntimeError: JIT C-source hash differs across MPI ranks.Symptom looked like a JIT non-determinism bug. Root cause was a parallel-unsafe accessor with no documented replacement. This PR adds the replacement.
Changes
src/underworld3/discretisation/discretisation_mesh.py:Mesh.get_mean_radius()method (28-line addition). Docstring explicitly warns against falling back toself._radii.mean()and explains the JIT-leak mechanism so the trap doesn't get rebuilt in user scripts.@uw.collective_operationdecorator onget_min_radius,get_max_radius,get_mean_radius(consistency — the decorator existed and was used in swarm.py / mesh_variables.py but wasn't yet applied here).No other files changed. No API breakage.
Test plan
Annulus(cellSize=1/16).get_mean_radius()returns a sensible value (0.0328, between the existing min0.0247and max0.0375)mean_radiusto ~3e-5 (small float-order variation in cell-volume computation across partitions — fundamental, not a bug; parallel-CONSISTENT which is what matters for JIT)Underworld development team with AI support from Claude Code