Pin the spherical relaxation rate and the direction of the surface response (#496) - #660
Open
lmoresi wants to merge 3 commits into
Open
Pin the spherical relaxation rate and the direction of the surface response (#496)#660lmoresi wants to merge 3 commits into
lmoresi wants to merge 3 commits into
Conversation
test_1072's surface assertion is symmetric in sign:
assert 0.0 < np.abs(shape).max() <= 1.5 * np.abs(h_inf).max()
Flip the 3-D recovery and |shape| is unchanged, so it passes while the surface
moves the wrong way. #496 records exactly that: CI cannot catch a 3-D sign
error.
Starting from flat, the displacement IS the shape, so it must correlate
POSITIVELY with the equilibrium it is relaxing toward. Measured +0.995; a sign
error gives about -0.995, which the magnitude bound cannot see. Asserted at
> 0.9.
PARTIAL, and the docstring now says so. The other half of #496 -- the spherical
relaxation RATE -- is not pinned. The review measured a fitted decay rate of
0.0842 against a half-space Cathles rate rho*g/(2*eta*k), k = sqrt(6)/R, of
0.204: ratio 0.41, an O(1) shell correction below the half-space value and in
the physically correct direction. Asserting that band needs an
initial-topography relaxation setup, and no test in the suite has one -- every
free-surface test starts flat and is driven by a load. Building it hastily
would risk a test that passes for the wrong reason, which is the failure #496
exists to fix, so it is left for its own session with the numbers recorded in
the docstring.
The mean-free assertion above is also weak (it applies _surface_mean to a field
_demean'ed with the same weights, so it is close to true by construction), as
#496 notes. Left alone here: replacing it properly means checking the gauge
against an independently computed surface integral, which belongs with the rate
work.
Underworld development team with AI support from Claude Code
Closes the second half of #496. Every other free-surface test starts flat and is driven by a load, so nothing in the suite exercised the decay of an imposed topography: a rate regression was invisible to CI. This adds a Y20 relaxation on a constant-density shell under radial gravity, where the topographic self-load is the only driver, and asserts the fitted decay rate against the half-space Cathles rate rho*g/(2*eta*k), k = sqrt(6)/R. Measured ratio 0.528 at cell 0.35 and 0.516 at cell 0.25; np=1 and np=2 agree to five digits. Asserted band 0.45 to 0.62. The band catches a sign error, an order-of-magnitude error, and the half-space value itself being returned; it does NOT catch a 2x error in buoyancy_scale (measured 0.476), and the assertion message says so rather than implying more coverage than it has. The decay lands on a floor - the #431-class bias in the recovered h_inf, 16% of the initial amplitude at cell 0.35 and 11% at 0.25 - so the extracted rate depends entirely on how the floor is treated: the same record gives 0.064 with no floor, 0.110 with the floor free, 0.142 with the floor pinned to the last sample. The protocol (floor free) is therefore fixed in code and documented. This also means the docstring's previous 0.0842 / ratio 0.41 was not reproducible without its protocol, and that the earlier "the rate is not yet converged" reading was mostly the naive fit reacting to a changing floor. Also replaces this file's mean-free assertion, which had no content: free_surface.py:901 builds _h_inf as _demean(-_demean(h)) and _demean IS subtraction of _surface_mean, so the asserted quantity is zero by construction - measured 1e-18 against a 1e-8 threshold - and it stays zero even if the trace-mass weights are wrong, because both sides use the same weights. It is now the FE boundary integral of the P1 carrier, an independent implementation of the same integral, plus a negative control asserting the UNWEIGHTED nodal mean is NOT zero (2.5e-4 of max|h_inf|), so the check cannot pass by construction. The rank-local max and shape bounds in the end-to-end test are now reduced globally; they were rank-local diagnostics that could disagree across ranks. Measurements, negative controls, and the surface-array ordering bug that had to be fixed in the probe first (matching _field_rows against the LIVE _ring_coords after a deform re-sorts by deformed x and silently permutes the carrier fill; free_surface.py:211 warns of exactly this) are in ~/+Simulations/spherical_relaxation_rate_496/README.md. That bug is also present in the earlier ylm_relaxation.py probe the workstream-C numbers came from. Serial and np=2: 3 passed, ~55s. Underworld development team with AI support from Claude Code
lmoresi
changed the title
The rate test added in the previous commit does not run in CI. scripts/test.sh excludes test_106*py and test_107*py pending the #504 triage, so test_1072_free_surface_spherical.py has never been executed there - which means BOTH halves of #496 were still invisible to CI, the sign half included, and merging the previous commit would have closed the issue without closing the gap it exists to close. Pulls that one file forward out of the excluded group, the same way tests/parallel/test_1069_boundary_normal_parallel.py already is, and for the same kind of reason. It is level_2/tier_b, ~55s serial, and passes at np=1 and np=2. The rest of the group still awaits its triage decision. Also names the mesh in the band-failure message. gmsh triangulates differently across platforms - serial_reference.py:202 records 1417 vs 1395 cells for a spherical shell between hosts - so the Linux runner will not build the 585-cell mesh the band was measured on. The resolution study makes a failure unlikely (cell 0.35 -> 0.25, a far larger change, moved the ratio only 2.3%), but if the band does trip on another host, the message now carries the cell count, A0 and the floor, which is what distinguishes a platform mesh difference from a real regression. 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.
Closes #496, both halves. The first commit pinned the direction of the 3-D surface response; the second pins the rate, and replaces the mean-free assertion this PR previously flagged as weak.
The rate
Every free-surface test in the suite starts flat and is driven by a load, so nothing exercised the decay of an imposed topography and a rate regression was invisible to CI. The new test imposes a degree-2 bump on a constant-density shell under radial gravity — the topographic self-load is the only driver — and fits the modal amplitude decay against the half-space Cathles rate ρg/(2ηk), k = √6/R = 0.204.
Asserted band 0.45 to 0.62. It catches a sign error, an order-of-magnitude error, and the half-space value itself being returned. It does not catch a 2× error in
buoyancy_scale— measured at 0.476, still inside the band. The exponential update is only weakly sensitive to that scale, and a band tight enough to see it would sit within a few percent of the resolution spread and flake. The assertion message says this rather than implying coverage it does not have.The fit protocol is part of the answer
The decay lands on a floor — the #431-class bias in the recovered
h_inf— so the extracted rate depends entirely on how the floor is treated. On the same cell-0.35 record:So the 0.0842 / ratio 0.41 this PR previously quoted is not reproducible without also quoting its protocol. The test now fixes the protocol in code (floor free) and documents why it must not be changed casually.
This also revises the earlier reading that the rate is not converged. That came from fitted rates 8.19 (res 0.2) and 6.07 (res 0.15) under the floor-pinned fit — the most floor-sensitive of the three — on runs whose floors were 16.3% and 2.2%. With the floor free, the ratio moves only 0.528 → 0.516 from cell 0.35 to 0.25. The 26% was mostly the fit reacting to a changing floor. The floor is what converges strongly; the rate is close to converged, though two coarse resolutions do not establish that and the test says so.
The mean-free assertion
free_surface.py:901builds_h_inf = _demean(-_demean(h)), and_demeanis subtraction of_surface_mean, so the asserted quantity was zero by construction: measured 1e-18 against a 1e-8 threshold. It also could not see a wrong trace-mass weight, since both sides use the same weights.Replaced by the FE boundary integral of the P1 carrier — an independent implementation of the same integral, which does depend on the weights being right — plus a negative control asserting the unweighted nodal mean is not zero (2.5e-4 of max|h_inf|). Without that control, "the weighted mean vanishes" says nothing about the gauge.
Two rank-local diagnostics in the end-to-end test (the
h_infandshapemaxima) are now reduced globally.A bug in the probe, worth knowing about
The measurement first showed the initial modal amplitude differing 4.6% between np=1 and np=2 on a bit-identical mesh. It was the instrument, not the library:
_field_rowsre-sorts whatever coordinates it is handed, so matching against_ring_coords— documented atfree_surface.py:211as live, deformed geometry — sorts by deformed x, a different permutation from the reference-x order every surface array uses, and silently permutes the carrier fill. With rows taken from_surf_coordsthe amplitude is identical across partitions to 11 digits.The same bug is in
ylm_relaxation.py, the probe the original workstream-C numbers came from. It biases the amplitude but not the decay rate, so those rates are not wrong for that reason.Measurements, scripts and the negative controls:
~/+Simulations/spherical_relaxation_rate_496/README.md.Tests:
test_10723 passed serial and at np=2, ~55s.Underworld development team with AI support from Claude Code
Follow-up: the test did not actually run in CI
scripts/test.shexcludestest_106*pyandtest_107*pypending the #504 triage, sotest_1072_free_surface_spherical.pyhas never been executed in CI — which means both halves of #496 were still invisible there, the sign half included. Merging the first commit would have closed the issue without closing the gap it exists to close.The second commit pulls that one file forward out of the excluded group, the same way
tests/parallel/test_1069_boundary_normal_parallel.pyalready is. The rest of the group still awaits its triage decision.It also names the mesh in the band-failure message. gmsh triangulates differently across platforms (
serial_reference.py:202records 1417 vs 1395 cells for a spherical shell between hosts), so the Linux runner will not build the 585-cell mesh the band was measured on. A failure looks unlikely — cell 0.35 → 0.25 is a far larger change and moved the ratio only 2.3% — but if the band does trip on another host, the message now carries the cell count, A0 and the floor, which is what separates a platform mesh difference from a real regression.Unrelated CI failure on the first push
test_0053_hang_watchdog.py::test_watching_restores_the_previous_watchdogfailed with a crashed xdist worker (worker 'gw0' crashed). Re-running the same commit unchanged passed, so it is flaky, not deterministic — and it cannot be caused by this branch in any case, sincetest_1072was not being run at all at that point. Filed as #661: the failure mode is a crashed worker rather than an assertion, which is the same class as #638 and #639 in the same watchdog machinery.