Repository navigation
fix: float32 overflow returned NaN scores; drop the drop-in replacement claim - #6
Merged
Merged
Conversation
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
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.
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_predictlabelled every point an inlier — including five planted extreme outliers — becauseNaN < offsetis 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 overXalone — making the new points each other reference population — then applied the offset learned duringfit. 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.mdsaying a large k is "free" andculof.hsaying cost does not grow with k were both wrong.Stale exponents
The README still carried n^2.01 / n^1.13 while
docs/implementation.mdhad 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).