Repository navigation
fix(kpm): log per-frame homography rejections at debug, not error - #242
Merged
Merged
Conversation
Closes #241. Eight sites on the KPM search path logged at error level via arlog_e!. They are per-frame rejections that are expected at runtime, so they flooded the browser console with red errors for as long as the tracker sat in the SEARCHING state, burying genuine errors. CLAUDE.md section 2 reserves arlog_e! for misconfiguration, invalid inputs and bad wiring - things a caller should fix - and specifies arlog_d! for per-frame rejections. Demoted to arlog_d!: - homography.rs, preemptive_robust_homography: `num_points < SAMPLE_SIZE` and `no valid hypothesis after N trials`. RANSAC cannot fit a homography from fewer than 4 correspondences, so it discards the hypothesis and continues; this is the normal search path. These are the two sites named in the issue. - homography.rs, solve_positive_definite_system_8x8 (4 sites: non-SPD matrix, zero pivot, and zero diagonal in both substitution passes). Called from the Levenberg-Marquardt refinement loop at homography.rs:2051, where returning false simply ends refinement early. - math.rs, solve_null_vector_8x9_destructive (2 sites: rank-deficient and identity-orthogonalization failure). Its only runtime caller is the 4-point DLT inside RANSAC, so a hit means the drawn correspondences were degenerate. Audited the remaining 59 arlog_e! sites under crates/core/src/kpm/ against the section 2 table and left them at error level - they are genuine misconfiguration ("k must be > 0", "index not built", "id already exists", "num_octaves is 0") or, in math.rs and homography.rs beyond the #[cfg(test)] boundary, dual-mode parity assertions in test code. mat3_exp_pade's singular Pade denominator also stays at arlog_e!: its own message notes it should not happen for Lie weights of small norm. Doc comments at each demoted site now state why the level is arlog_d!, so the next reader does not "restore" them. Verified: cargo fmt --check, cargo clippy --workspace --all-targets --all-features -D warnings, cargo test --workspace --all-features (465 + 34 across the other targets), and a browser run of simple_video_nft_example.html pushing 16 frames through the full KPM detect + AR2 track path - the marker is acquired and the console is clean, where the same run previously produced a continuous flood. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
codecov/patch failed at 0.00% on this branch: every line the previous commit touched is a log statement inside an error branch, and none of those branches were exercised by the suite. The lines were already uncovered before the change - swapping arlog_e! for arlog_d! simply made them show up as a diff with no coverage. Adds five tests that reach the rejection branches directly: - preemptive_robust_homography with 3 points (below SAMPLE_SIZE) - preemptive_robust_homography where every draw is degenerate, so no hypothesis ever survives - solve_positive_definite_system_8x8 with a negative leading entry (the `s < 0.0` non-SPD guard) - solve_positive_definite_system_8x8 with A = L*Lt for a lower-triangular L whose last diagonal is zero, which reaches the zero-diagonal guard in forward substitution rather than the earlier zero-pivot guard - solve_null_vector_8x9_destructive with an all-zero design matrix Each test was verified to reach its intended branch by temporarily swapping the arlog_d! calls for panic! and confirming the reported line numbers were distinct (1448, 1525, 1942, 1953, 1970). Also corrects a claim in the previous commit's doc comment. A design matrix built from four coincident correspondences does NOT trip the rank-deficient guard - the column-pivoted Gram-Schmidt still produces a null vector. The doc now says the branch fires only on a genuinely degenerate constraint matrix, and points at the actual caller behaviour instead. Two guards remain uncovered because they are unreachable: solve_positive_definite_system_8x8's zero-diagonal check in back substitution re-tests the same eight diagonal entries that forward substitution already rejected, and orthogonalize_identity_8x9's zero-pivot check cannot fire because 8 basis vectors cannot annihilate all 9 identity rows in R^9. Both are left in place as defensive code; see the PR description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit added tests for the rejection branches but could not have fixed codecov/patch, because the eight lines in the diff ARE the arlog_d! calls, and those lines are structurally uncoverable without a logger installed. Evidence from the tarpaulin report for bd77b04 (crates/core/src/kpm/freak/homography.rs): 1952: if l_jj == 0.0 { hits 2 1953: arlog_d!("... zero pivot ..."); hits 0 1954: return false; hits 1 The branch was taken and the `return false` ran, but the log line above it reported zero hits. With no logger installed, log's level filter short-circuits and the instructions attributed to the macro call site never execute. This affects arlog_e! identically, which is part of why project coverage sits at 57.15%. Also confirmed from the same report: tarpaulin stops instrumenting at line 2235, exactly the `#[cfg(test)] mod tests` boundary, so in-file test code contributes nothing to patch coverage. Adding tests alone could never have moved the number. Fix: promote the existing capture logger init in arlog.rs to a crate-internal `tests::ensure_logger_installed()` and call it from the tests that exercise an arlog_d! branch. With a logger installed at Trace the macro body runs and the line is counted. This cannot disturb the arlog tests' own exact-count assertions: CaptureLogger already discards records from any thread that is not the active capture target, and init_capture() now delegates to the same one-time init. Expected patch coverage: 6 of the 8 coverable diff lines. The remaining two are unreachable guards documented in the PR description (solve_positive_definite_system_8x8's back-substitution check re-tests the diagonals forward substitution already rejected; orthogonalize_identity_8x9's zero-pivot check cannot fire for 8 basis vectors in R^9). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit be26c28. The hypothesis was that arlog_*! lines report as uncovered because, with no logger installed, log's level filter short-circuits and the macro body never runs. Installing a logger did not change the result: patch coverage stayed at 0/8. The tarpaulin report for be26c28 shows why the hypothesis was wrong. Every guard IS executed by the tests added in 749e2ba - each `return false` records a hit - while only the macro lines read zero: 1969: if l_ii == 0.0 { hits 2 1970: arlog_d!( hits 0 1974: return false; hits 1 And homography.rs:517 is an arlog_e! line that IS covered, by an ordinary test with no logger installed. So a logger was never the missing ingredient. The actual cause is tarpaulin's attribution of macro expansions: of 203 arlog_*! call sites in instrumented files it instruments only 45, and marks just 17 as hit. Which ones land is not something the code under test controls. Reverting leaves the tests from 749e2ba in place - they are worth keeping regardless, since they pin rejection behaviour that had no coverage at all - and drops the logger machinery, which added indirection to arlog.rs without achieving anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tarpaulin cannot reliably attribute coverage to arlog_*! call sites, so any
PR whose diff is mostly logging fails codecov/patch with no change the
contributor can make to raise the number.
Measured on this PR: of the 203 arlog_*! call sites in instrumented files,
tarpaulin instruments only 45 and marks just 17 as hit. This PR's diff is
eight arlog_*! lines and nothing else, so it scored a hard 0/8.
The failure mode, from this PR's own tarpaulin report:
1969: if l_ii == 0.0 { hits 2
1970: arlog_d!( hits 0
1974: return false; hits 1
The branch executed and the return recorded a hit; the macro line between
them reported zero. Five tests were added (749e2ba) that reach these guards
for the first time, and the number did not move, because the diff lines are
the macro lines. Installing a logger was tried and reverted (be26c28 /
be73077) after the report showed an arlog_e! line elsewhere is covered by an
ordinary test with no logger installed.
project stays a blocking status, so a real drop in overall library coverage
is still caught. Only patch becomes informational: codecov still reports the
number on every PR, it just no longer blocks.
This extends the precedent already documented at the top of this file from
#180 PR 4.5, where a cleanup PR failed the same gate for a related reason.
A proper fix would be moving the coverage job off tarpaulin (cargo-llvm-cov
attributes macro expansions correctly); that is out of scope here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Closes #241.
Eight sites on the KPM search path logged at error level via
arlog_e!. They are per-frame rejections that are expected at runtime, so they flooded the browser console with red errors for as long as the tracker sat in theSEARCHINGstate — burying genuine errors.CLAUDE.md §2 reserves
arlog_e!for "misconfiguration, null/invalid inputs, bad wiring — things a caller should fix" and specifiesarlog_d!for "per-frame rejections … expected at runtime, noisy, debug-level only." All eight are squarely in the second category.No behavioural change: every site already returned
false/Errand the callers already handled it. This changes the log level only.Demoted to
arlog_d!homography.rs—preemptive_robust_homography:num_points < SAMPLE_SIZEhomography.rs—preemptive_robust_homography:no valid hypothesis after N trialshomography.rs—solve_positive_definite_system_8x8(4 sites: non-SPD matrix, zero pivot, zero diagonal in both substitution passes)homography.rs:2051; returningfalsesimply ends refinement early.math.rs—solve_null_vector_8x9_destructive(2 sites: rank-deficient, identity-orthogonalization failure)homography.rs:1149), so a hit means the drawn correspondences were degenerate.Doc comments at each demoted site now state why the level is
arlog_d!, so the next reader does not "restore" them.Audit of the remaining sites
The issue asked for a sweep of the other
arlog_e!calls undercrates/core/src/kpm/. All 67 were reviewed; the other 59 stay at error level:clustering.rs,hough.rs,matcher.rs,visual_database.rs,gaussian_pyramid.rs,pyramid.rs:"k must be > 0","index not built","tree not built","id already exists","num_octaves is 0","bytes_per_feature must be > 0". Exactly whatarlog_e!is for.#[cfg(test)]boundary inmath.rs(line 1430) andhomography.rs(line 2236) are dual-mode parity assertions, not runtime paths.mat3_exp_padesingular Padé denominator stays atarlog_e!— its own message notes this should not happen for Lie weights of small norm, so it is a real anomaly rather than an expected rejection.solve_linear_system_2x2/solve_symmetric_linear_system_3x3/solve_tridiagonal_destructivehave no runtime callers outside tests today, so they keep the stricter level as general-purpose helpers where a singular matrix is a caller error.Verification
cargo fmt --all -- --checkcleancargo build --all-featurescleancargo clippy --workspace --all-targets --all-features -- -D warningscleancargo test --workspace --all-featuresgreen (465 core + 34 across the other targets)simple_video_nft_example.htmlagainst the static test image, pushing 16 frames through the full KPM detect + AR2 track path. The marker is acquired (found/lostcycling as before) and the console is clean, where the same run previously produced a continuous flood ofpreemptive_robust_homography: num_points (3) < 4.Related