Summary
matmath.invert_3x3 divides by the determinant without checking it. For a singular matrix it returns a matrix of NaN and ±Infinity with no signal to the caller — no return value, no exception, no flag.
Found while writing the edge-case tests for #87 (PR #115).
Reproduction
const A = new jsfeatNext.matrix_t(3, 3, F32C1);
A.data.set([1, 2, 3, 2, 4, 6, 1, 1, 1]); // rows 1 and 2 are dependent
jsfeatNext.matmath.mat3x3_determinant(A); // 0
const Ai = new jsfeatNext.matrix_t(3, 3, F32C1);
jsfeatNext.matmath.invert_3x3(A, Ai);
// Ai.data -> -Infinity, Infinity, NaN, Infinity, -Infinity, NaN, -Infinity, Infinity, NaN
Not a regression
Original jsfeat produces byte-identical output for the same input — verified against the vendored oracle, including the Infinity/NaN placement. Inherited, and invisible to the parity suite because both sides agree.
Why it is worth addressing
NaN propagates. A degenerate homography or a collapsed affine fit yields a singular matrix, and the poisoned inverse then flows into whatever consumes it — multiply_3x3, a warp, a pose estimate — surfacing much later as a blank frame or a wild transform, far from the actual cause.
Contrast with linalg.lu_solve, which does report failure on a singular system (returns 0) and whose caller can react. invert_3x3 gives the caller nothing to check.
This is the same family as #102, where svd_invert returned a silently wrong pseudo-inverse for non-square input and was changed to throw.
Options
(a) Return a status, 1 on success and 0 when the determinant is zero (or below a small epsilon), matching lu_solve's existing convention in this codebase. The function currently returns void, so this is an additive change — existing callers ignoring the result keep working, and the numeric output need not change at all. This is the option I would pick.
(b) Throw, as #102 did for svd_invert. Consistent with the precedent, but harsher: invert_3x3 sits in hot paths inside motion_model, where a degenerate sample during RANSAC is expected and currently just produces a bad model that the estimator discards. Throwing there would need call-site handling.
(c) Leave the numbers, document the behaviour. Cheapest, and arguably defensible since NaN is at least detectable by a caller who thinks to look — unlike a plausible-looking wrong answer. But nothing in the API hints that looking is necessary.
Worth checking option (b)'s blast radius before choosing: motion_model.ts calls invert_3x3 during model fitting, and degenerate point samples are routine there.
Test coverage
tests/properties/edge-cases.test.ts pins the current behaviour under a CHARACTERIZATION: label, asserting the determinant is 0 and that the result contains non-finite entries. It is written to be replaced by whichever fix lands.
Summary
matmath.invert_3x3divides by the determinant without checking it. For a singular matrix it returns a matrix ofNaNand±Infinitywith no signal to the caller — no return value, no exception, no flag.Found while writing the edge-case tests for #87 (PR #115).
Reproduction
Not a regression
Original jsfeat produces byte-identical output for the same input — verified against the vendored oracle, including the
Infinity/NaNplacement. Inherited, and invisible to the parity suite because both sides agree.Why it is worth addressing
NaNpropagates. A degenerate homography or a collapsed affine fit yields a singular matrix, and the poisoned inverse then flows into whatever consumes it —multiply_3x3, a warp, a pose estimate — surfacing much later as a blank frame or a wild transform, far from the actual cause.Contrast with
linalg.lu_solve, which does report failure on a singular system (returns 0) and whose caller can react.invert_3x3gives the caller nothing to check.This is the same family as #102, where
svd_invertreturned a silently wrong pseudo-inverse for non-square input and was changed to throw.Options
(a) Return a status,
1on success and0when the determinant is zero (or below a small epsilon), matchinglu_solve's existing convention in this codebase. The function currently returnsvoid, so this is an additive change — existing callers ignoring the result keep working, and the numeric output need not change at all. This is the option I would pick.(b) Throw, as #102 did for
svd_invert. Consistent with the precedent, but harsher:invert_3x3sits in hot paths insidemotion_model, where a degenerate sample during RANSAC is expected and currently just produces a bad model that the estimator discards. Throwing there would need call-site handling.(c) Leave the numbers, document the behaviour. Cheapest, and arguably defensible since
NaNis at least detectable by a caller who thinks to look — unlike a plausible-looking wrong answer. But nothing in the API hints that looking is necessary.Worth checking option (b)'s blast radius before choosing:
motion_model.tscallsinvert_3x3during model fitting, and degenerate point samples are routine there.Test coverage
tests/properties/edge-cases.test.tspins the current behaviour under aCHARACTERIZATION:label, asserting the determinant is 0 and that the result contains non-finite entries. It is written to be replaced by whichever fix lands.