Skip to content

fix: float32 overflow returned NaN scores; drop the drop-in replacement claim - #6

Merged
Aminsed merged 1 commit into
mainfrom
fix/review-claims
Jul 28, 2026
Merged

Aminsed merged 1 commit into
mainfrom
fix/review-claims

Conversation

@Aminsed

@Aminsed Aminsed commented Jul 28, 2026

Copy link
Copy Markdown
Owner

From external review. The overflow case was flagged as unconfirmed — it reproduces, and it is the most serious item here.

Confirmed bug: silent "no outliers"
Coordinates around 1e20 contain no NaN or infinity, so they passed validation, then overflowed when squared. Every selection key tied at infinity and every score came back NaN. fit_predict labelled every point an inlier — including five planted extreme outliers — because NaN < offset is False. An outlier detector reporting nothing wrong when it has failed is the worst available failure mode. Now guarded by a magnitude precondition (checked in logs so the check cannot itself overflow) and a non-finite backstop on the output.

Not a drop-in replacement
score_samples(X)/predict(X) recomputed transductive LOF over X alone — making the new points each other reference population — then applied the offset learned during fit. That is not scikit-learn novelty semantics and it returned a plausible-looking array. The argument is removed, so it is now a TypeError. The claim is gone from the README, the package description and the class docstring, replaced with what is actually covered and an explicit list of what is not.

k complexity corrected
Only the selection scan is k-independent. Neighbour storage is O(n·k) and both the density and score kernels loop over all k, so docs/usage.md saying a large k is "free" and culof.h saying cost does not grow with k were both wrong.

Stale exponents
The README still carried n^2.01 / n^1.13 while docs/implementation.md had the re-baselined n^1.94 / n^1.31 over n ≥ 50,000. My omission when re-baselining.

Benchmark now enforces agreement
It computed relative error and rank agreement, printed them, and asserted nothing. It now raises, so "a speedup on a wrong answer is not a speedup" is executable.

Development Status: Beta → Alpha, since nothing in CI executes a kernel.

50 tests pass (45 + 5 new regressions).

@Aminsed
Aminsed merged commit 4ca7d21 into main Jul 28, 2026
4 of 5 checks passed
@Aminsed
Aminsed deleted the fix/review-claims branch July 28, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant