Inverse-distance weights use the actual distance; p defaults to 1 (#427)#448
Inverse-distance weights use the actual distance; p defaults to 1 (#427)#448lmoresi wants to merge 1 commit into
Conversation
The kd-tree returns SQUARED distances, and the weighting used them
directly:
weights = 1 / (epsilon + distance_n)**p # distance_n is r^2
so the decay was r^(-2p), not the r^(-p) the argument is named and
documented for. At the old default p=2 the scheme was inverse FOURTH
power -- close to nearest-neighbour, not the "inverse distance (squared)"
the docstring claimed. The retained old_* implementations took sqrt
explicitly, so the semantics changed silently at a rewrite and nothing
caught it: no test pinned the exponent, and the only accuracy test used
rtol=5e-2, loose enough that 1/r^2 and 1/r^4 both pass.
Two changes, maintainer decision:
- take the square root, so p applies to the actual distance;
- default p 2 -> 1, so the default is genuinely inverse distance.
epsilon stays 1e-12 but now floors r rather than r^2, so its scale is the
one it reads as (it was an effective 1e-6 length floor).
Pinned by test: two sources at distance 1 and 2 give a weight ratio of
exactly 2**p for p in (1, 2, 3), and the default gives 2. The exponent
had no test at all before.
Measured effect on a linear field through the swarm-proxy stencil
(nnn=dim+1): 2D 8.79e-2 at p=1 vs 8.77e-2 at p=2; 3D 2.05e-1 vs 2.04e-1.
The choice of p barely matters -- what mattered was that p meant
something other than its name, and that order=0 is now the fallback
rather than the primary path.
Call sites updated to stop hard-coding p=2: SwarmVariable.rbf_interpolate
now lets the default apply, MeshVariable.rbf_interpolate's own p default
follows to 1, and the nnn=1 checkpoint call drops a positional p it never
used.
test_interpolation_matrix_agrees_with_the_value_path now sweeps nnn and p
as well as order. It previously pinned order only and hard-coded p=2 on
the value side, so it compared two different weightings the moment the
default changed -- a gap flagged in this PR series' own review.
level_1 567 passed, level_2 319 passed.
Underworld development team with AI support from Claude Code
Adversarial reviewThis changes numbers for every existing
|
Closing the gap I flagged: measured on curved and discontinuous fieldsThe review above said the linear-field metric was the worst possible probe for an exponent change and that I had not run a better one. Running it now. Max error,
So the conclusion survives contact with a better probe, and mildly favours the change:
That removes the substantive objection. The remaining ones stand: this still changes numbers for |
Fixes #427.
The defect
KDTree.query/find_closest_n_pointsreturn squared distances, and the weighting used them directly:So the decay was
r^(-2p), not ther^(-p)the argument is named and documented for. At the old defaultp=2the scheme was inverse fourth power — close to nearest-neighbour — not the "inverse distance (squared)" the docstring claimed.The retained
old_*implementations tooksqrtexplicitly, so this changed silently at a rewrite. Nothing caught it: no test pinned the exponent, and the only accuracy test usedrtol=5e-2, loose enough that1/r²and1/r⁴both pass.The change (maintainer decision)
papplies to the actual distance.p2 → 1, so the default is genuinely inverse distance.epsilonstays1e-12but now floorsrrather thanr², so its scale is the one it reads as — it was an effective1e-6length floor.Verification
Two sources at distance 1 and 2 from the target; interpolating a field that is 1 at the near point and 0 at the far one recovers the weight ratio directly:
pw₁/w₂2^pThe exponent had no test at all before; it does now, including one pinning the default at
p=1.How much it actually changes
Linear field through the swarm-proxy stencil (
nnn=dim+1), max relative error:p=1p=2The choice of
pbarely matters. What mattered was thatpmeant something other than its name — and that since #430,order=0is the fallback rather than the primary path, so this is a correctness-of-contract fix rather than an accuracy one.Call sites
Stopped hard-coding
p=2:SwarmVariable.rbf_interpolatelets the default apply,MeshVariable.rbf_interpolate's ownpdefault follows to 1, and thennn=1checkpoint call drops a positionalpit never used (nnn=1short-circuits before any weighting).Test gap closed
test_interpolation_matrix_agrees_with_the_value_pathnow sweepsnnnandpas well asorder. It previously pinnedorderonly and hard-codedp=2on the value side — so it compared two different weightings the moment the default changed, and duly failed here. That gap was flagged in this series' own adversarial review; this is it biting.Regression
tier_a/tier_b: 567 passed, 0 failedtier_a/tier_b: 319 passed, 0 failedUnderworld development team with AI support from Claude Code