Reviewing someone's ML code

Setup#

pip install scikit-learn

Verified with scikit-learn 1.7.2, numpy 1.26.4, Python 3.10, CPU. Runs in about five seconds. This is a script you can paste into any review, unchanged.

The four checks, as code#

The dataset below has one planted leak, of the exact kind that reaches production regularly: a column recorded after the outcome it is supposed to predict.

import numpy as np
from sklearn.ensemble import RandomForestClassifier
from sklearn.metrics import roc_auc_score
from sklearn.model_selection import train_test_split

rng = np.random.default_rng(0)
n = 600
X = rng.normal(size=(n, 6))
y = (X[:, 0] + 0.5*X[:, 1] + rng.normal(0, 0.8, n) > 0).astype(int)
X = np.column_stack([X, y * 0.9 + rng.normal(0, 0.3, n)])   # column 6 leaks the label
names = [f"f{i}" for i in range(6)] + ["days_since_outcome"]

def review(X, y, names):
    Xtr, Xte, ytr, yte, itr, ite = train_test_split(
        X, y, np.arange(len(y)), test_size=0.3, random_state=0)

    print("check 1 - do any rows appear in both splits?")
    print(f"          overlap: {len(set(itr) & set(ite))} rows")

    print("check 2 - can one feature alone predict the label?")
    for j, name in enumerate(names):
        auc = roc_auc_score(y, X[:, j])
        flag = "  <-- LEAK?" if max(auc, 1-auc) > 0.90 else ""
        print(f"          {name:20s} single-feature AUC {max(auc, 1-auc):.3f}{flag}")

    m = RandomForestClassifier(n_estimators=60, random_state=0).fit(Xtr, ytr)
    real = roc_auc_score(yte, m.predict_proba(Xte)[:, 1])
    print(f"check 3 - real score: test AUC {real:.3f}")

    shuffled = rng.permutation(ytr)          # the known weight on the scale
    m2 = RandomForestClassifier(n_estimators=60, random_state=0).fit(Xtr, shuffled)
    fake = roc_auc_score(yte, m2.predict_proba(Xte)[:, 1])
    print(f"check 4 - shuffled-label score: test AUC {fake:.3f}  (must be near 0.5)")

review(X, y, names)

check 1 - do any rows appear in both splits?
          overlap: 0 rows
check 2 - can one feature alone predict the label?
          f0                   single-feature AUC 0.829
          f1                   single-feature AUC 0.655
          f2                   single-feature AUC 0.532
          f3                   single-feature AUC 0.502
          f4                   single-feature AUC 0.536
          f5                   single-feature AUC 0.512
          days_since_outcome   single-feature AUC 0.985  <-- LEAK?
check 3 - real score: test AUC 0.987
check 4 - shuffled-label score: test AUC 0.475  (must be near 0.5)

Four checks, five seconds, and the review is already better than most.

Reading the four answers#

Check 1 passed, and that is worth knowing. Zero overlap means the split itself is clean. When it is not zero, nothing else on the page matters — see train-test contamination.

Check 2 found the bug. days_since_outcome scores 0.985 on its own. No single ordinary feature does that. The name is the second clue: a column measured in "days since the outcome" cannot exist at prediction time, because the outcome has not happened yet. This is the most common leak in industry, and target leakage is the full treatment.

Check 3 is a smell, not a verdict. An AUC of 0.987 on a noisy behavioural problem should stop a reviewer cold. It is not proof of a bug — some problems really are easy — but it moves the burden of proof onto the author. Too good to be true lists the questions to ask next.

Check 4 passed, and passing is the useful outcome here. 0.475 is close to the 0.5 expected from scrambled answers, so the evaluation harness is sound: the split, the metric and the label alignment are wired correctly. That matters, because it tells the reviewer the 0.987 is a real leak in the data rather than a broken measurement. Two very different bugs, distinguished by one extra fit.

The review checklist that follows from this#

Ask these in the pull request, in this order. The first three take under a minute each.

1. Where does the split happen, and is it the first thing that touches the data? Any preprocessing fitted before the split leaks test statistics into training (preprocessing leakage).
2. Is the split random when it should be grouped or time-ordered? One customer with 40 rows across both sides is group leakage; a random split on a forecasting problem is temporal leakage. A random train_test_split is the default and it is wrong for most real datasets.
3. For every feature: would this value exist, with this value, at the moment of prediction? Ask it column by column. This single question catches more production failures than every other item combined.
4. Is the metric the one the decision uses? Accuracy on a 2% positive class is a well-dressed lie (macro, micro and weighted averaging).
5. Is the test set used anywhere except the final number? Threshold picking, feature selection and early stopping on the test set are all test-set overfitting.
6. Are the numbers in the description reproducible from this branch? Ask for the seed and the command. If neither exists, the result is an anecdote (reproducing your own result).
7. Then review it as ordinary code — naming, dead code, error handling, tests. This part still matters; it is not where the expensive bugs are.

How to say it#

Review comments on ML work go wrong in a predictable way, so two conventions help.

Ask about the data, not about the author. "What is days_since_outcome measured from?" is a question anybody can answer without defensiveness. "This is leaking" is a verdict that invites an argument before the facts are in.

Separate blocking findings from suggestions, explicitly. A leak blocks. A variable name does not. Marking every comment with one of the two words saves an entire round trip, and it stops a genuine leak from being lost in a list of eleven style notes.

Common mistakes#

Approving because the tests pass. Unit tests check that code does what it was written to do. Every bug in this lesson is code doing exactly what it was written to do.

Reviewing the diff instead of the pipeline. A three-line diff can leak, because the leak lives in the interaction between the new line and the split that happens sixty lines above it. Read the data's path from load to metric, every time.

Skipping the review because the score improved. An improvement is the strongest reason to review, not the weakest. In ML the direction of a bug's effect is usually upward.

Running the checks and not saving them. Paste the four checks into tests/ so the next pull request gets them for free. A review habit that depends on the reviewer remembering is a habit that ends on a busy day — the same argument data validation makes about the gate.

Try it yourself#

Make check 4 fail on purpose. In review, replace roc_auc_score(yte, ...) with roc_auc_score(ytr[:len(yte)], ...) so the scorer compares test predictions against training labels. Notice that check 3 still returns a number that looks plausible, and only check 4 exposes it. That is what a broken scale looks like from the inside.

What to learn next#

• Target leakage — the bug check 2 was built to find.
• Too good to be true — what to do when check 3 fires.
• Reproducing a failure you only see in production — the review that happens after the model ships.