Make LOCI honor contamination and keep k as a parameter - #707
Conversation
LOCI stored the constructor argument k directly into self.threshold_ and never called _process_decision_scores(), so contamination was accepted but completely ignored when deriving labels_. Store k on the instance, use it in the score loop where it belongs, and let the base class compute threshold_ from contamination like every other detector. Fixes yzhao062#194
|
I reviewed this and the fix looks correct. CI is green, and I also verified it locally with the fixture in The ROC being identical is expected and worth stating explicitly: this changes how Your read of the root cause is right: Two things before it can be merged:
Thanks for this — it's been a latent bug since LOCI landed. |
|
Thanks for the thorough check — good to see the contamination sweep and the clone path reproduce on your side too, and agreed that the unchanged ROC is the right thing to call out: I've retargeted the base branch to On the draft status: I'm keeping it as a draft until Mohit signs off on it — that's my standing rule for anything opened under this account, not a sign of pending work. The change is complete from my side and I'm not planning further commits. I'll flip it to ready-for-review as soon as he confirms, which should be within the day. One note on scope while it's in front of you: this covers the |
|
Marked ready for review — thanks for waiting, and for taking the time to reproduce the sweep independently. Nothing changed since your review: the base is on |
LOCIaccepts acontaminationargument and documentsthreshold_as "based oncontamination... then_samples * contaminationmost abnormal samples", butthe value never reaches the thresholding logic. In
__init__the other parameter,k, is written straight intoself.threshold_:and
fit()then labels against that samek, socontaminationis dead weight.Sweeping it changes nothing:
There's a second consequence of the same line. Because
kis consumed instead ofstored,
LOCIhas nokattribute, so scikit-learn'sget_paramsfalls back toNoneand a cloned estimator is silently broken:The existing
test_model_clonedoesn't catch this because it only clones, it neverfits the clone.
What this changes
Three small edits, all in
loci.py:__init__storesself.k = kinstead of overwritingthreshold_.self.k— that's the outlier cutoffkis documented to be, and it's the only place it was ever really used.
fit()callsself._process_decision_scores()instead of hand-rollinglabels_/_mu/_sigma. That's the base-class helper every other detector inthe package uses (
lof,cof,sod, ...), and it derivesthreshold_fromcontaminationvia the documented percentile rule.The stale
k=Nonein the class docstring's example repr is updated tok=3tomatch the actual default.
Scores are untouched —
decision_functionand_calculate_decision_scorecomputeexactly what they did before for a given
k. Only the threshold used to turn thosescores into binary labels changes, which is the reported bug.
Testing
Ran the module's suite in a clean venv (numpy/scipy/scikit-learn/numba,
pip install -e .):That includes three new cases added to
TestLOCI, each of which fails onmasterbefore the change:
test_k_is_stored_as_parameter—AttributeError: 'LOCI' object has no attribute 'k'test_contamination_controls_labels— all three contamination values produced theidentical outlier fraction
test_threshold_matches_contamination—ACTUAL: 0.14 DESIRED: 0.25After the change the same sweep tracks the requested rate:
Detection quality is unchanged on the standard synthetic benchmark (train ROC 0.959,
test ROC 0.963 at
contamination=0.1), andkstill visibly drives the scores(
k=1/3/10give mean scores 0.519/0.506/0.240), confirming it kept its real role.Also ran
pyod/test/test_base.py,test_data.pyandtest_utility.py(39 passed)since
fit()now routes through the shared base-class path.flake8 --max-line-length=127on both touched files reports the same 25 pre-existing violations as on
master—no new ones.
Fixes #194