fix(anomaly): stop PredictiveAnomalyDetection.score_one from mutating the model - #1970
fix(anomaly): stop PredictiveAnomalyDetection.score_one from mutating the model#1970JayeshSuryavanshi wants to merge 3 commits into
Conversation
… the model score_one updated the dynamic MAE/variance that back the anomaly threshold, so scoring a point changed the detector and repeated scoring of the same point returned different values. Move the threshold update into learn_one, computing the prediction error before the predictive model learns each observation, so score_one is side-effect free. Scores over the usual score-then-learn loop are unchanged.
Merging this PR will not alter performance
Comparing 
Footnotes
|
| def test_score_one_does_not_mutate_state(): | ||
| """`score_one` must be side-effect free. | ||
|
|
||
| Regression test: `score_one` used to update the dynamic MAE/variance backing the threshold, | ||
| so scoring a point changed the model and repeated scoring of the same point returned | ||
| different values (cf. the analogous issue #1331 for `anomaly.LocalOutlierFactor`). | ||
| """ | ||
| model = _fit(anomaly.PredictiveAnomalyDetection(warmup_period=0)) | ||
|
|
||
| mae_n = model.dynamic_mae.n | ||
| var_n = model.dynamic_se_variance.n | ||
| snapshot = pickle.dumps(model) | ||
|
|
||
| probe_x, probe_y = {"x": 30.0}, 999.0 | ||
| scores = [model.score_one(probe_x, probe_y) for _ in range(5)] | ||
|
|
||
| # Scoring left every statistic untouched... | ||
| assert model.dynamic_mae.n == mae_n | ||
| assert model.dynamic_se_variance.n == var_n | ||
| assert pickle.dumps(model) == snapshot | ||
| # ...so it is idempotent. | ||
| assert len(set(scores)) == 1 |
There was a problem hiding this comment.
Actually here I'd prefer it we added a global test in the checks to make sure score_one does not mutate any anomaly detector whatsoever
There was a problem hiding this comment.
Good call — done in 79e9202. Added checks.anomaly.check_score_one_does_not_mutate: it learns a stream, snapshots the model (via pickle.dumps), calls score_one, and asserts the state is unchanged. It's registered for every anomaly detector — both AnomalyDetector and SupervisedAnomalyDetector — and passes all of them (GaussianScorer, HalfSpaceTrees, LODA, LocalOutlierFactor, OneClassSVM, StandardAbsoluteDeviation, PredictiveAnomalyDetection); reverting the PAD fix makes it fail, so it genuinely guards the whole class going forward.
One structural note: I yield it directly with its own dataset rather than appending it to the generic dataset_checks, because supervised anomaly detectors (currently just PredictiveAnomalyDetection) aren't wired into those — the common checks call predict_one, which score_one-based detectors don't implement. Wiring supervised anomaly detectors into the generic checks felt like a bigger, separate change, so I kept this check self-contained; happy to do that as a follow-up PR if you'd like.
I also dropped the now-redundant per-detector mutation test and kept the learn_one-maintains-the-threshold-statistics test in test_pad.py.
…ly detector Per @MaxHalford's review, add a framework-level checks.anomaly.check_score_one_does_not_mutate that verifies score_one leaves any anomaly detector (supervised or not) untouched, rather than a PAD-specific test. It is yielded directly with its own dataset because supervised anomaly detectors are not wired into the generic (predict_one-based) dataset_checks. It runs on all anomaly detectors and catches the PAD regression when the fix is reverted. Drop the now-redundant per-detector mutation test; keep the PAD learn_one-maintains-statistics test.
|
Hi @MaxHalford, can you please review it when you get time :) |
| if isinstance(model, AnomalyDetector): | ||
| dataset_checks.append(anomaly.check_roc_auc) | ||
|
|
||
| # score_one must never mutate an anomaly detector (supervised or not). | ||
| # Yielded directly with its own dataset rather than via dataset_checks: | ||
| # supervised anomaly detectors are not wired into the generic | ||
| # (predict_one-based) dataset_checks, but their score_one must be | ||
| # side-effect free too. | ||
| if isinstance(model, (AnomalyDetector, SupervisedAnomalyDetector)): | ||
| from river import datasets as _anomaly_datasets | ||
|
|
||
| yield _wrapped_partial( | ||
| anomaly.check_score_one_does_not_mutate, | ||
| dataset=list(_anomaly_datasets.CreditCard().take(500)), | ||
| ) |
There was a problem hiding this comment.
You can simplify here. First of all the import datasets is weird. Just use the one that's already imported. Also no need for the comment, it's too verbose. Finally, you can simplify and run both the ROC AUC test and the new test for all anomaly detectors, including supervised ones.
There was a problem hiding this comment.
Done in 6262189. Both check_roc_auc and check_score_one_does_not_mutate now run for every anomaly detector, supervised ones included, and I dropped the verbose comment.
On the import: I kept it function-local (like _yield_datasets already does) because a module-level from river import datasets risks a circular import, but I dropped the alias. I also couldn't fold the two anomaly checks into the generic dataset_checks for supervised detectors, since those go through predict_one, which SupervisedAnomalyDetectors don't implement, so both checks are yielded here over a shared CreditCard stream. Happy to restructure if you had a cleaner shape in mind.
| x, y = first | ||
| snapshot = pickle.dumps(anomaly_detector) | ||
| if supervised: | ||
| anomaly_detector.score_one(x, y) | ||
| else: | ||
| anomaly_detector.score_one(x) | ||
|
|
||
| assert pickle.dumps(anomaly_detector) == snapshot, f"score_one mutated {anomaly_detector!r}" |
There was a problem hiding this comment.
Couldn't you test this at each learn_one step? That would be stricter (and therefore a better test)
There was a problem hiding this comment.
Good call, done in 6262189. It now snapshots and asserts no mutation at each step of the stream, interleaved with learn_one.
One wrinkle the stricter version surfaced: PAD's predictive-model pipeline lazily memoises _inference_transformers / _last_step_cached on the first predict, which a naive per-step pickle flags (that is caching, not learning). I prime it with a single warm-up score before the loop so the check targets real state changes. It runs on all 7 anomaly detectors including PAD, and a short prefix over the stream keeps the per-step pickling fast.
… check Per review: - Run both check_roc_auc and check_score_one_does_not_mutate on every anomaly detector, including supervised ones (PredictiveAnomalyDetection). Both now handle the supervised score_one(x, y) / learn_one(x, y) signature via _supervised. - Tidy the registration: drop the aliased datasets import and the verbose comment. - Make the mutation check stricter: it now asserts score_one leaves the model untouched at each step of the stream, interleaved with learn_one, after a one-time warm-up score that primes the pipeline's lazy prediction cache (memoisation, not learning). A short prefix keeps the per-step pickling fast.
2 participants
What
anomaly.PredictiveAnomalyDetection.score_oneupdated the dynamic MAE/variance statistics that back its anomaly threshold. Scoring a point therefore changed the detector: repeatedscore_onecalls on the same point returned different values, and scoring without learning silently shifted the threshold. This is the same class of bug fixed forLocalOutlierFactor(#1331, #1929), where the convention is thatscore_onemust be side-effect free —PredictiveAnomalyDetectionwas the only remaininganomalydetector whosescore_onemutated state.Fix
The threshold statistics are now maintained by
learn_one, which computes the prediction error before the predictive model learns each observation.score_oneonly reads them, so it no longer mutates the model. A small private_predicthelper is shared by both methods.Behavior is preserved for the documented score-then-learn loop: the full 144-score sequence over the docstring's
AirlinePassengersexample is byte-for-byte identical before and after, and the doctest output is unchanged (0.05329236123455621).Tests
Added
river/anomaly/test_pad.py: it assertsscore_oneleavesdynamic_mae/dynamic_se_varianceand a full pickle snapshot untouched (so it is idempotent), and thatlearn_onenow maintains the threshold statistics. Both tests fail onmainand pass with this change; the 14 automated estimator checks forPredictiveAnomalyDetectionstill pass.ruff check,ruff format --check, andmypy --strictare clean.