Hard baselines instead of drift detectors, and pin cell_size so a redefinition fails on the day

[lmoresi]

Maintainer ruling (2026-09-14): a test asserts against a hard baseline, or it
is not a test. A test loose enough to absorb a change in a default does not
fail when the default changes — it drifts inside its own margin and breaks later,
somewhere else, for reasons that are hard to trace back.

The case study

#734. #692 redefined mesh.cell_size(); nothing failed; the Nitsche penalty
γμ/h moved 43%; the Zhong spherical-shell benchmark slid from 0.2% to 2.4%
— still inside its 5% tolerance — and only broke months later on one platform's
triangulation, three layers from the cause.

Why the existing tests missed it

cell_size had two tests, and between them they missed the case that changed:

test	covers	baseline
test_regular_square_cell_size_keeps_global_radius	regular quad box	exact (√2/8) — and there the two measures happen to agree
test_cell_size_matches_own_vertices_and_tracks_deform	simplices	none — its oracle independently computes the same definition, so it stays true if the definition moves on both sides

#692 changed the simplex path.

The test that would have caught it on the day

# congruent right-isosceles cells of legs h: vertices (0,0), (h,0), (0,h)
# RMS distance to the centroid is exactly 2h/3
assert |cell_size − 2h/3| < 1e-12

Plus the relationship, which is itself exact:

mesh	cell_size()	get_min_radius()	ratio
2-D quad	h√2/2	same	1.0000
2-D regular simplex	2h/3	√2·h/3	√2

The Nitsche penalty scales with that ratio, so pinning it means neither measure
can move silently.

Negative-controlled against the real 1.745× shift — both new tests fail, and
both name the quantity rather than a downstream symptom:

closed_form: failed as intended -> cell_size on congruent legs-0.25 cells is 0.2908...
ratio:       failed as intended -> on tensor cells the two measures must coincide; ratio 1.745

The docstring was false

add_nitsche_bc said the two measures "coincide on a uniform mesh". True on a
regular quad box — presumably where it was checked — and false on a uniform
simplex mesh, which is what every free-slip and fault model uses. Corrected
in the implementation comment and the user-facing parameter docs.

Four comparisons removed rather than relocated

#729 moved them to tier C. That was the wrong fix — in every case the hard
baseline was already present and the ratio was discarding it:

• test_1070 computed an absolute error against the prescribed datum, then
  asserted errors["strong"] < 0.5 * errors["penalty"]. Now asserts the error
  directly. The ratio also hid which side moved: strong went 1.06e-2 → 1.73e-2
  between platforms while penalty barely changed.
• test_0066, test_0773 — baselines already asserted in sibling tests,
  so the comparisons added nothing actionable.
• test_1060 asserted the weak constraints must stay inaccurate
  (leak > 1e-5) — it would fail if Nitsche improved. Its exact half (an
  essential BC holds v·n to machine precision) is a hard baseline and is now a
  tier B contract; the weak-path leaks are printed, not gated.

Docs

Charter §8 leads with the baseline requirement and says why it must be tight
enough to fail the moment a default changes. TESTING-RELIABILITY-SYSTEM.md
records that C1 characterisations are rare by design — reach for one only after
showing no baseline exists.

55 passed, 1 skipped across the five files.

Underworld development team with AI support from Claude Code

[Copilot]

🟡 Changes recommended

Unresolved issues affect MPI safety, CI gating, documentation consistency, and baseline tightness.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR strengthens regression coverage for mesh-size semantics, replaces comparison-based checks with direct baselines, and updates related documentation.

Changes:

• Adds closed-form cell_size() and radius-ratio tests.
• Replaces method comparisons with direct baseline assertions.
• Updates Nitsche documentation and testing guidance.

File summaries

File	Reviewed change and outstanding notes
tests/test_1070_free_surface_plume.py	Adds a direct baseline. CI execution, metric wording, baseline tightness, and unused penalty work need correction.
tests/test_1060_nitsche_freeslip.py	Adds an essential-BC contract; the test is not currently included in the CI merge gate.
tests/test_0773_surface_smoother.py	Removes smoother comparison characterization.
tests/test_0066_integration_point_slcn.py	Removes the method comparison; the module description still claims the unverified comparison.
tests/test_0010_cell_size_geometry.py	Adds closed-form geometry tests; MPI empty-rank reductions, ratio wording, and public docstrings need correction.
src/underworld3/cython/petsc_generic_snes_solvers.pyx	Corrects Nitsche documentation; the simplex ratio needs qualification and other public copies need updating.
docs/developer/UW3_STYLE_CHARTER.md	Strengthens baseline guidance, but conflicts with the retained Tier C policy.
docs/developer/TESTING-RELIABILITY-SYSTEM.md	Clarifies Tier C characterization-test usage.

Review details

Suppressed comments (9)

docs/developer/UW3_STYLE_CHARTER.md:121

• This makes every non-baseline assertion a benchmark rather than a test, but the unchanged Tier C section below still defines C1 characterisation tests—including method comparisons—as suite tests, and TESTING-RELIABILITY-SYSTEM.md:75-100 preserves that fallback. Please reconcile the absolute rule with the supported C1 policy (or remove the C1 definition), otherwise contributors have contradictory guidance.

- **Every test asserts against a HARD BASELINE.** An analytic solution, a published
  value, a closed-form geometric quantity, a conservation identity, an exactness
  property. If a failure cannot name what is broken, it is not a test — it is a
  drift detector, and it belongs in a benchmark rather than the suite.

src/underworld3/cython/petsc_generic_snes_solvers.pyx:6762

• This implementation comment repeats the same overgeneralisation: the sqrt(2) value is a 2-D right-isosceles example, not a universal property of uniform simplex meshes. Keep the exact fixture result, but state that other simplex geometries can have a different ratio.

        # The two coincide on TENSOR cells only. On a uniform SIMPLEX mesh --
        # which is what the free-slip and fault models are built on -- they
        # differ by exactly sqrt(2): on congruent right-isosceles cells of legs
        # h, cell_size is 2h/3 and get_min_radius is sqrt(2)h/3. The penalty
        # gamma*mu/h moves with that, so the two settings are NOT interchangeable

src/underworld3/cython/petsc_generic_snes_solvers.pyx:6659

• The correction here is not propagated to the other public copies: src/underworld3/discretisation/discretisation_mesh.py:3253-3255 still says a uniform mesh reduces to get_min_radius, and docs/docstrings/inventory.json:4272 still says the two measures coincide on a uniform mesh. Both are false for the uniform simplex case documented here, so regenerate/update those user-facing docstrings with this change.

            The two coincide on **tensor** cells only. On a uniform **simplex**
            mesh they differ by exactly :math:`\sqrt{2}` — for congruent
            right-isosceles cells of legs :math:`h`, :meth:`Mesh.cell_size` is
            :math:`2h/3` while :meth:`Mesh.get_min_radius` is
            :math:`\sqrt{2}h/3` — so the penalty :math:`\gamma\mu/h` differs
            between the two settings on the simplex meshes the free-slip and
            fault models use. See ``tests/test_0010_cell_size_geometry.py``.

tests/test_0010_cell_size_geometry.py:120

• quad_local is rank-local, so .min() raises on a starved MPI rank before the other ranks can complete the collective. The module advertises MPI use, and get_min_radius() explicitly handles empty partitions; reduce a local minimum with an inf identity across ranks instead.

    quad_ratio = float(quad_local.min()) / quad.get_min_radius()

tests/test_0010_cell_size_geometry.py:133

• simplex_local is rank-local, so .min() raises on a starved MPI rank before the other ranks can complete the collective. The module advertises MPI use, and get_min_radius() explicitly handles empty partitions; reduce a local minimum with an inf identity across ranks instead.

    ratio = float(simplex_local.min()) / simplex.get_min_radius()

tests/test_0010_cell_size_geometry.py:103

• This sentence is self-contradictory: the measures do not coincide by sqrt(2); on simplices their ratio is sqrt(2), while tensor cells have ratio 1. Please state the two cases explicitly so the test documents the contract it asserts.

    """The two mesh-size measures coincide on TENSOR cells only, by sqrt(2).

tests/test_0010_cell_size_geometry.py:104

• This new regression establishes that uniform simplex cell_size() and get_min_radius() differ, but the public Mesh.cell_size() docstring still says the opposite ("On a uniform mesh ... reduces to global get_min_radius", src/underworld3/discretisation/discretisation_mesh.py:3253-3255). Please update that public docstring too; otherwise the API documentation contradicts the contract pinned here.

def test_cell_size_and_min_radius_agree_on_tensor_cells_but_not_simplices():
    """The two mesh-size measures coincide on TENSOR cells only, by sqrt(2).

tests/test_0066_integration_point_slcn.py:124

• Removing the only nodal-vs-integration-point comparison leaves the module docstring's claim that the integration-point scheme is “at least as accurate as” nodal and preserves the peak unverified. Please update that opening description to list only the exact-departure and absolute-accuracy contracts that remain.
  tests/test_1070_free_surface_plume.py:330
• After removing the rival-method assertion, errors['penalty'] is no longer consumed by any assertion; the fixture still runs four penalty solves/advances and this print only reports it. That doubles this level-2 test's expensive work without gating any penalty behavior. Either remove the penalty case/diagnostics or move the comparison to an explicit tier-C test.

    print(f"datum error: strong={errors['strong']:.2e} penalty={errors['penalty']:.2e}")

• Files reviewed: 8/8 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.