Skip to content

Fix pipeline data leakage, merge fan-out, and repo hygiene - #1

Open
skylarliu1 wants to merge 3 commits into
hkbuttar:mainfrom
skylarliu1:fix/pipeline-leakage
Open

Fix pipeline data leakage, merge fan-out, and repo hygiene#1
skylarliu1 wants to merge 3 commits into
hkbuttar:mainfrom
skylarliu1:fix/pipeline-leakage

Conversation

@skylarliu1

Copy link
Copy Markdown
Collaborator

What this fixes

1. Data leakage in every supervised notebook (primary)

All 7 classifiers merged the unsupervised artifacts (dbscan_df.csv, gmm_df.csv,
kmeans_clusters.csv, renttherunway_lda_topics.csv) into df before
train_test_split(). Those artifacts were produced by models (DBSCAN, GMM,
K-means, LDA) fit on the entire RTR dataset, so test-row cluster labels and
topic vectors were informed by the test set.

Fix: new src/split.py defines one frozen split (row-level, stratified on
fit_label, test_size=0.20, random_state=42). Every Phase 1 notebook now fits
its scaler / model / PCA / model-selection sweep on the training partition only
and merely transforms every row:

model how test rows are assigned
GMM, K-means predict / predict_proba
DBSCAN (no predict) nearest fitted core sample within eps, else -1
LDA Dictionary + LdaModel built from train reviews; test reviews scored with get_document_topics (inference only). Train-only coherence sweep now picks k=9 (was k=11 on the full corpus).

Every Phase 2 notebook takes its train/test partition from df["split"] instead of
a fresh train_test_split.

2. Row-count fan-out bug

dbscan_df.csv was transaction-grained but keyed only on user_id (~70k duplicate
keys). df.merge(dbscan, on="user_id") fanned 192,544 → 1,156,140 rows, which
is where the "test set of 266,286 samples" came from. The other three merges added
smaller fan-out from 266 genuine duplicate (user_id, item_id) transactions in
renttherunway_clean.csv.

Fix: each artifact is de-duplicated on its merge key at the source
(dbscan_df.csv → one row per user_id; the others → unique (user_id, item_id[, fit, fit_label])),
and each supervised notebook re-asserts len(df) == 192_544 after the merges.
Clean 20% test set is now 38,509 rows.

3. Repo hygiene

  • New .gitignore (venvfit/, .DS_Store, __pycache__/, Data/, Models/).
  • New requirements.txt (pinned to the project venv).
  • git rm --cached Data/Raw/, Data/Processed/ (regenerable via
    download_data.py / data_cleaning.py / the notebooks) and Models/
    (regenerable by re-running Phase 2; bagging_simple_classifier.joblib alone is
    160 MB and was the repo's only Git-LFS entry — its .gitattributes rule is
    removed). Local files are kept; this is a normal commit, not a history
    rewrite. venvfit/ stays in history as-is per the brief.
  • README.md: added a "Reproducing the Pipeline" run-order section.
  • Fixed a pre-existing figure/data path bug in DBSCAN, K-means, and
    Model_Comparison (../../../, exposed by the earlier repo restructure).

Impact on results

Removing the leakage (and the fan-out) collapses every model to near the
majority-class base rate:

model leaky acc / macro-F1 fixed acc / macro-F1
CART 0.73 / 0.67 0.50 / 0.38
Random Forest 0.76 / 0.70 0.53 / 0.40
Bagging 0.77 / 0.72 0.52 / 0.40
KNN 0.91 / 0.87 0.69 / 0.34
Kernel SVM 0.85 / 0.78 0.52 / 0.40
Linear SVM 0.68 / 0.39 0.74 / 0.29
Naive Bayes 0.46 / 0.41 0.42 / 0.35

Model_Comparison now shows the rich RTR feature set is worse than the simple
3-feature (weight/age/height) baseline for every model
(macro-F1 Δ −0.23 to
−0.40). Its "KNN on rich features is the strongest overall performer" conclusion no
longer holds.

Follow-ups for reviewers (not done here)

  1. Write-up prose. Every model's markdown write-up + the LDA and Model_Comparison
    write-ups have a ⚠️ banner with the fixed headline numbers, but the narrative
    paragraphs, per-class tables, and confusion-count callouts still describe the
    leaky results and need a human rewrite.
  2. Same user in train and test. Body measurements are constant per user_id, so
    a user with rows on both sides still leaks (weaker form). Kept a plain row-level
    split here to stay comparable; consider GroupShuffleSplit on user_id.
  3. predict vs argmax(predict_proba). Model_Comparison scores the RTR models
    from the saved probability files, which disagrees with each notebook's own
    predict() for the class_weight="balanced" models (e.g. Kernel SVM: 0.52 vs
    0.74 acc). Pre-existing; worth reconciling.
  4. Data/ and Models/ are no longer in git — run src/download_data.py
    src/data_cleaning.py → the notebooks (order in the README) to regenerate.

How it was verified

src/split.py → 154,035 train / 38,509 test (exactly 20%, stratified). All 11
notebooks (4 unsupervised + 7 supervised) + Model_Comparison were executed
end-to-end on venvfit with a venvfit kernel; each supervised notebook prints
train rows: 154,035 / test rows: 38,509 and its merge-fan-out assertion passes.

🤖 Generated with Claude Code

skylarliu1 and others added 3 commits August 28, 2026 11:46
- .gitignore: venvfit/, .DS_Store, __pycache__, .ipynb_checkpoints, Data/Raw/, Data/Processed/
- requirements.txt: packages from README Requirements, pinned to venvfit versions
- git rm --cached Data/Raw + Data/Processed (regenerable via download_data.py /
  data_cleaning.py / the notebooks) to relieve Git-LFS bandwidth. Local files kept.
- src/split.py: canonical frozen train/test split shared by all notebooks
  (row-level, stratified on fit_label, test_size=0.20, random_state=42)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e keys

DBSCAN, GMM, K-means, LDA now load the frozen split from src/split.py and fit
their scaler / model / PCA / coherence-sweep on the training rows only. Every
row (train + test) is then *transformed*:
  - GMM / K-means: predict / predict_proba
  - DBSCAN (no predict): nearest fitted core sample within eps, else -1
  - LDA: dictionary + LdaModel built from train reviews; test reviews scored
    with get_document_topics (inference only). Train-only coherence sweep now
    selects k=9 (was k=11 on the full corpus).

Merge-key hygiene (fixes the row fan-out that produced the bogus
"266,286-sample test set"):
  - dbscan_df.csv  -> one row per user_id
  - gmm_df.csv     -> unique (user_id, item_id)
  - kmeans_clusters.csv -> unique (user_id, item_id)
  - renttherunway_lda_topics.csv -> unique (user_id, item_id, fit, fit_label)

Also fixes a pre-existing figure-path bug (Notebooks/Figures/... -> Figures/...)
exposed by the repo restructure. Regenerated Figures/Unsupervised_Outputs/*.
README: add "Reproducing the Pipeline" run order + train/test-discipline note.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ures

All 7 classifiers (+ Model_Comparison) now:
  - load df via src/split.load_rtr_with_split()
  - de-duplicate each unsupervised artifact on its merge key, then assert the
    merged table is still 192,544 rows (no fan-out)
  - derive X_train/X_test from df["split"] instead of a fresh train_test_split

Effect of removing the leakage (test set 266,286 -> 38,509 rows):

  model          leaky acc / macroF1   ->   fixed acc / macroF1
  CART           0.73 / 0.67           ->   0.50 / 0.38
  Random Forest  0.76 / 0.70           ->   0.53 / 0.40
  Bagging        0.77 / 0.72           ->   0.52 / 0.40
  KNN            0.91 / 0.87           ->   0.69 / 0.34
  Kernel SVM     0.85 / 0.78           ->   0.52 / 0.40
  Linear SVM     0.68 / 0.39           ->   0.74 / 0.29
  Naive Bayes    0.46 / 0.41           ->   0.42 / 0.35

Model_Comparison now shows the rich RTR feature set is WORSE than the simple
3-feature baseline for every model (macro-F1 delta -0.23 to -0.40); its
"KNN on rich features wins" conclusion no longer holds. Each write-up cell gets
a banner with the fixed numbers; the prose still needs a human rewrite.

Also: fix Model_Comparison's Data path (../ -> ../../, restructure bug).

Repo hygiene: stop tracking Models/ (regenerable; bagging_simple_classifier
alone is 160 MB and was the lone Git-LFS entry). Added to .gitignore, removed
the .gitattributes LFS rule. Local files kept.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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