Claude finished @Yeraze's task —— View job

PR Review — fix(position-estimation): large uncertainty for weight-dominated single-anchor solves (#3616)

• Gather context
• Read changed files
• Analyze implementation
• Post review feedback

Summary: Well-reasoned fix with clean implementation. The root-cause analysis in the PR description is accurate, the math is correct, and the tests are comprehensive. A few minor observations below.

positionEstimationService.ts

The core fix (lines 148–158) is mathematically sound.

const confidence = Math.min(1, Math.max(0, nEff - 1));
const uncertaintyKm = Math.max(
  MIN_UNCERTAINTY_KM,
  DEFAULT_SINGLE_ANCHOR_KM * (1 - confidence) + statisticalKm * confidence,
);

The linear blend from nEff = 1 (full radio-range default) to nEff ≥ 2 (pure statistical) is a principled and transparent choice. Using the Kish effective sample size is appropriate here.

One subtle edge case to consider: What happens when nEff is between 1 and 2 and statisticalKm > DEFAULT_SINGLE_ANCHOR_KM? For example, if two anchors are very far apart (say 50 km), the statistical estimate would be large, and with confidence = 0.5, the blend would compute:

5 * 0.5 + 50 * 0.5 = 27.5 km

This is lower than the pure statistical estimate (50 km), so the blend would actually under-report uncertainty in that transition zone for very-wide anchor sets. This is probably acceptable in practice (widely separated anchors likely still constrain the location reasonably), but it's worth noting as a design choice rather than a bug.

statisticalKm floor uses Math.max(nEff, 1) (line 148):

const statisticalKm = Math.max(MIN_UNCERTAINTY_KM, rmsKm / Math.sqrt(Math.max(nEff, 1)));

Since used.length > 0 and wSum > 0 are checked at line 131, nEff = wSum²/w²Sum will be ≥ 1 (by Cauchy-Schwarz) whenever there's at least one valid observation, so the Math.max(nEff, 1) guard is defensive but redundant. Not a problem.

positionEstimationService.test.ts

The new tests are well-structured and directly target the failure modes:

SNR ordering test (line 104–113): Good regression guard. The hardcoded expected values (0.0631, 1, 10) match 10^(snrDb/10) exactly and serve as an independent cross-check.

Lone anchor test (line 120–126): Correctly pins the exact value (toBe(5) for DEFAULT_SINGLE_ANCHOR_KM). Note that this test uses a specific SNR value (-12 dB), but the outcome (exactly 5 km) holds for any SNR on a lone anchor — the test could optionally add a second variant with snrDb: 10 to make that invariant explicit. Minor suggestion, not a blocker.

Regression case (line 133–143): The toBeCloseTo(10.0, 2) tolerance (±0.005 degrees) is slightly looser than the SNR bias test (line 91 uses implicit tolerance). Fine, since the real assertion is the uncertainty check. The uncertaintyKm > 4 threshold (line 142) is a reasonable fuzzy bound rather than pinning an exact value that could drift with formula tweaks.

Balanced cluster test (line 148–157): Good non-regression guard. uncertaintyKm < 1 for a cluster spread ≈ 0.005° × cos(lat) ≈ 0.55 km is plausible; the test validates that the fix doesn't inflate confidence for genuinely multi-anchor solves.

Missing test scenario: There's no test for exactly nEff == 2.0 (the crossover point where confidence = 1 and the uncertainty transitions to pure-statistical). This would confirm the boundary is clean. Again, minor — the existing tests bracket it well from both sides.

Code Quality

• The implementation is clean, minimal (25 lines changed), and contained entirely within solveNodePosition.
• Documentation comments are thorough and clearly explain the "why".
• No raw SQL, no multi-source violations, no new dependencies introduced.
• The old if (nEff <= 1) branch that could silently bypass the radio-range default is cleanly eliminated.

Verdict

Approve. The fix correctly addresses the root cause (uncertainty underestimation in weight-dominated cases), the blend formula is well-chosen, tests cover the key scenarios including the regression case from the issue. The minor points above are non-blocking suggestions.