fix(motion_estimator): add find_homography, the missing refit layer RANSAC/LMEDS never had

[kalwalt]

Summary

ransac()/lmeds()'s own doc comments claimed a post-convergence refit over all inliers that neither ever performed — both are, and remain, byte-for-byte faithful to jsfeat.motion_estimator and OpenCV's RANSACPointSetRegistrator/LMeDSPointSetRegistrator, which don't refit either. The refit lives one layer up, in OpenCV's cv::findHomography, and jsfeat never ported that layer.

This adds it as Option B (decided on #185): a new, composed find_homography() entry point — ransac()/lmeds() are untouched.

find_homography(params, kernel, from, to, count, model, mask?, method = "ransac", max_iters = 1000):

1. Runs ransac() or lmeds() to find a robust minimal-sample model + inlier mask.
2. Refits the model over the full inlier set via one extra kernel.run().
3. Recomputes the mask against the refit model, so model and mask describe the same transform.
4. Falls back to the pre-refit model/mask on a degenerate (kernel.run() <= 0) or collapsed (post-refit inliers < model_points) refit.

Generic over MotionKernel, so it works with both homography2d and affine2d kernels without new code.

Fixes found via /code-review (two-axis) before merge

The Spec-axis sub-agent caught that step 3's reclassification always used params.thresh, but that field is documented as "ignored by lmeds" and callers correctly pass 0 for it — meaning find_inliers' cutoff collapsed to zero for the "lmeds" method and the refit silently no-op'd. Fixed by deriving lmeds's own robust threshold (same formula lmeds() itself uses) instead of reusing params.thresh for that branch, and strengthened the test to assert a refit actually occurred rather than only that the fallback doesn't crash.

Testing

• npm test: 317/317 passing, including the untouched tests/parity/motion_estimator.test.ts (parity oracle for ransac/lmeds against vendored jsfeat — confirms neither's behavior changed).
• New tests/properties/find_homography.test.ts (10 tests): noise-free refit convergence (measured, not qualitative — see inline comment), outlier rejection + mask/model agreement, degenerate-input fallback, both "ransac" and "lmeds" methods, optional mask param, insufficient-points failure path.
• npx tsc --noEmit, prettier --check, check-license-headers.mjs: clean.

Closes #185, #188.

[qodo-code-review]

PR Summary by Qodo

Add post-inlier refit via find_homography

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Adds an opt-in robust estimation layer that refits models using all detected inliers.
• Reclassifies masks after refitting, with safe fallback for degenerate or collapsed results.
• Preserves existing RANSAC/LMEDS parity and adds focused numerical property coverage.

Diagram

graph TD
    A["Point Pairs"] --> B{"Method"}
    B -->|ransac| C["RANSAC Model"] --> E["Inlier Refit"]
    B -->|lmeds| D["LMeDS Model"] --> E
    E --> F{"Refit Valid"}
    F -->|yes| G["Reclassified Mask"]
    F -->|no| H["Original Result"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Refit inside RANSAC and LMeDS

• ➕ Makes every robust-estimation call return an all-inlier fit.
• ➕ Avoids adding another public entry point.
• ➖ Breaks behavioral parity with jsfeat and OpenCV’s point-set registrators.
• ➖ Changes established APIs for callers that depend on minimal-sample results.
• ➖ Would require modifying and revalidating both estimator implementations.

2. Add nonlinear LM refinement

• ➕ Could further reduce reprojection error after the linear all-inlier fit.
• ➕ More closely matches OpenCV’s complete homography refinement pipeline.
• ➖ Adds substantial numerical complexity and review risk.
• ➖ Requires convergence, stability, and failure-handling policies beyond this fix.
• ➖ Expands scope beyond the missing linear refit layer.

Recommendation: Keep the composed find_homography approach. It restores the missing caller-level refit while preserving byte-for-byte estimator parity, centralizes shared behavior for homography and affine kernels, and safely defers nonlinear LM polishing to separate work.

Files changed (2) +402 / -4

Enhancement (1) +133 / -4

motion_estimator.tsAdd robust all-inlier model refitting +133/-4

Add robust all-inlier model refitting

• Introduces find_homography as a composed RANSAC/LMEDS entry point that refits the selected model over all initial inliers and recomputes its mask. It derives an LMeDS-specific robust threshold and falls back to the original model and mask when refitting is degenerate or loses too many inliers. Existing estimator documentation now accurately states that those methods return minimal-sample models without refitting.

src/motion_estimator/motion_estimator.ts

Tests (1) +269 / -0

find_homography.test.tsCover refit accuracy, consistency, and fallback behavior +269/-0

Cover refit accuracy, consistency, and fallback behavior

• Adds deterministic property tests for RANSAC and LMeDS refitting, outlier rejection, model-mask agreement, optional masks, minimal samples, and insufficient input. The tests also verify that zero-threshold LMeDS performs a real refit using its derived robust cutoff.

tests/properties/find_homography.test.ts

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Buffer releases lack finally ✗ Dismissed 📘 Rule violation ☼ Reliability

Description

The new method borrows shared-cache buffers and releases them only along normal return paths,
without try/finally protection. If an injected kernel or another operation throws, one or more
buffers remain checked out and can exhaust or degrade the shared pool.

Code

src/motion_estimator/motion_estimator.ts[R463-464]

+        const mask_buff = this.cache.get_buffer(count);
+        const own_mask = new matrix_t(count, 1, JSFEAT_CONSTANTS.U8C1_t, mask_buff.data);

Evidence

Rule 2965982 requires every shared-buffer acquisition to have a matching release on all paths,
especially error paths. The method acquires its first buffer at lines 463-464 and additional buffers
at lines 487-492, while releases occur later without any finally guard around intervening
estimator and kernel calls.

Rule 2965982: Release all shared buffer pool allocations with a matching put_buffer call
src/motion_estimator/motion_estimator.ts[463-474]
src/motion_estimator/motion_estimator.ts[487-524]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shared-cache buffers borrowed by `find_homography` are not guaranteed to be returned when an operation throws.

## Issue Context
The method invokes caller-supplied kernel operations after borrowing buffers. Restructure buffer ownership with nested or centralized `try`/`finally` blocks so every successfully acquired buffer is returned exactly once on success, early return, and exception paths.

## Fix Focus Areas
- src/motion_estimator/motion_estimator.ts[463-528]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Failure returns stale mask ✓ Resolved 🐞 Bug ≡ Correctness

Description

When the underlying estimator returns false, find_homography copies own_mask even though
several RANSAC/LMEDS failure paths never initialize it. The caller can therefore receive arbitrary
mask values left in the shared cache instead of the underlying estimator's existing failure
behavior.

Code

src/motion_estimator/motion_estimator.ts[472]

+            if (mask) own_mask.copy_to(mask);

Evidence

The new mask wraps reused cache storage without clearing it and is copied on failure. The underlying
estimators return before writing a mask for insufficient input and initial subset-generation
failures, while the cache returns existing nodes without clearing their contents.

src/motion_estimator/motion_estimator.ts[463-474]
src/motion_estimator/motion_estimator.ts[198-198]
src/motion_estimator/motion_estimator.ts[249-255]
src/motion_estimator/motion_estimator.ts[317-317]
src/motion_estimator/motion_estimator.ts[371-379]
src/cache/cache.ts[108-118]
src/matrix_t/matrix_t.ts[117-136]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`find_homography` copies its cache-backed internal mask to the caller when `ransac` or `lmeds` returns `false`. Some estimator failure paths never write that internal mask, so stale pooled bytes become externally visible.

## Issue Context
Both estimators may return before initializing the mask, including when there are too few points or the first valid subset cannot be generated. Preserve the established failure behavior by not copying an unwritten mask, or explicitly initialize and document a deterministic failure mask.

## Fix Focus Areas
- src/motion_estimator/motion_estimator.ts[463-474]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. math singleton instantiated ✗ Dismissed 📘 Rule violation ≡ Correctness

Description

The new refit path constructs the stateless math algorithm with new math() instead of invoking
the provided singleton namespace. This violates the required singleton usage pattern and creates an
unnecessary algorithm instance per refit.

Code

src/motion_estimator/motion_estimator.ts[507]

+                    const median = new math().median(err, 0, count - 1);

Evidence

Compliance rule 2965977 prohibits constructing stateless algorithm modules and requires singleton
namespace calls. The added line directly constructs math solely to invoke median.

Rule 2965977: Do not instantiate algorithm modules; use provided singleton namespace functions
src/motion_estimator/motion_estimator.ts[507-507]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new LMEDS refit path instantiates the stateless `math` algorithm with `new math()` instead of using its provided singleton.

## Issue Context
Algorithm modules such as `math` must be accessed through the singleton namespace rather than constructed at call sites. Replace the new construction while preserving the median calculation.

## Fix Focus Areas
- src/motion_estimator/motion_estimator.ts[507-507]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 27 rules

✅ Skills: license-header-adder

Review mode: ⚖️ Balanced: This adds a new generic robust-estimation/refit path with method-specific thresholds, masking, fallback behavior, buffer management, and public API semantics, creating genuine correctness risk but not enough independent complexity to require redundant extended review.

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!