Follow-up from the PR #1437 (ruview-unified, ADR-273..282) deep review: full write-up at
https://gist.github.com/ruvnet/89795f3c4b8ea166cff5ac35ae4c7651
The functional bugs found in review are already fixed and merged (panics, NaN
corruption, entity-conflation, wrong center-freq for real captures, missing
task-field validation, untested purpose branches). This issue tracks three
remaining items that are about test rigor, not correctness — none block
anything, but the tests currently claim to verify more than they do:
-
heads.rs's factorized_pose_resists_room_shortcut_leakage never
exercises the actual encoder. It hand-constructs synthetic 2-3 dim
vectors where the anti-leakage property (room_x included vs excluded) is
baked in by the test author, not learned or verified against
RfEncoder::encode_content's real pooled-token representation. The PR's
cited MPJPE numbers (0.0003 m vs 0.2534 m) describe the head's algebra on
that toy input, not the real 24-dim-token -> 128-dim z pipeline. Worth
an end-to-end version that trains the actual encoder + RePos head on
synthetic multi-room windows and checks the room-shortcut leakage there.
-
gaussian/gain.rs's empty_map_returns_exact_friis never invokes the
erf/Beer-Lambert path it's named for. With no Gaussians, optical_depth
short-circuits to tau = 0.0 before the loop that would call
line_integral/erf ever runs, so the test is really "float
multiplication by an exact 1.0." The physically meaningful check
(line_integral_matches_numeric_quadrature, 1e-6 vs 1D quadrature) is
legitimate and already exists — this item is just about not citing the
trivial test as if it validated the closed-form solution.
-
"Learns a 6.1 dB obstruction in 20 observations" is 20 repeats of one
measurement converging one scalar unknown, since tau = occ * integral
is exactly linear in the single spawned Gaussian's occupancy. It's a
correct convergence check of the observe_link update rule (each call is
a damped Newton step, residual shrinks geometrically), but reads as
evidence of learning from diverse observations, which it isn't. Consider
a version with several distinct links/measurements resolving genuinely
different unknowns, if that scenario is meant to be demonstrated.
None of these are blocking — they're about making the test names and PR
claims match what the tests actually exercise. Tracked on
chore/pr1437-followup-test-rigor (currently empty, branched off main).
Follow-up from the PR #1437 (
ruview-unified, ADR-273..282) deep review: full write-up athttps://gist.github.com/ruvnet/89795f3c4b8ea166cff5ac35ae4c7651
The functional bugs found in review are already fixed and merged (panics, NaN
corruption, entity-conflation, wrong center-freq for real captures, missing
task-field validation, untested purpose branches). This issue tracks three
remaining items that are about test rigor, not correctness — none block
anything, but the tests currently claim to verify more than they do:
heads.rs'sfactorized_pose_resists_room_shortcut_leakageneverexercises the actual encoder. It hand-constructs synthetic 2-3 dim
vectors where the anti-leakage property (room_x included vs excluded) is
baked in by the test author, not learned or verified against
RfEncoder::encode_content's real pooled-token representation. The PR'scited MPJPE numbers (0.0003 m vs 0.2534 m) describe the head's algebra on
that toy input, not the real 24-dim-token -> 128-dim
zpipeline. Worthan end-to-end version that trains the actual encoder + RePos head on
synthetic multi-room windows and checks the room-shortcut leakage there.
gaussian/gain.rs'sempty_map_returns_exact_friisnever invokes theerf/Beer-Lambert path it's named for. With no Gaussians,
optical_depthshort-circuits to
tau = 0.0before the loop that would callline_integral/erfever runs, so the test is really "floatmultiplication by an exact 1.0." The physically meaningful check
(
line_integral_matches_numeric_quadrature, 1e-6 vs 1D quadrature) islegitimate and already exists — this item is just about not citing the
trivial test as if it validated the closed-form solution.
"Learns a 6.1 dB obstruction in 20 observations" is 20 repeats of one
measurement converging one scalar unknown, since
tau = occ * integralis exactly linear in the single spawned Gaussian's occupancy. It's a
correct convergence check of the
observe_linkupdate rule (each call isa damped Newton step, residual shrinks geometrically), but reads as
evidence of learning from diverse observations, which it isn't. Consider
a version with several distinct links/measurements resolving genuinely
different unknowns, if that scenario is meant to be demonstrated.
None of these are blocking — they're about making the test names and PR
claims match what the tests actually exercise. Tracked on
chore/pr1437-followup-test-rigor(currently empty, branched offmain).