Repository navigation
Fix evaluation leakage & methodology; de-duplicate model; honest reporting - #2
deepanshumody wants to merge 1 commit into
Conversation
The headline "ROC-AUC ~0.95 on 1FUF" was in-sample: 1FUF is in the training list (data/nonredundantRNA.txt) and the splits shuffled candidate *sites*, so near-identical grid points from one structure leaked across train/val/test. This corrects the methodology and the claims, and adds tests. Core fixes - Structure-level split (rna_splits.py): group by PDB id, never by site; hold whole structures out of the corpus. Wired into all trainers + GNN-DTI. - Imbalance-aware: pos_weight in BCEWithLogitsLoss; report PR-AUC + enrichment + precision/recall@Youden, not ROC-AUC alone (rna_metrics.py). - Reproducibility: single seed via rna_seed.py. - Focal loss: apply the (1-p_t)^gamma modulation per example (was reduced to a scalar first, collapsing it to a scaled BCE). - Atom features: 'Mg' is now an explicit symbol with an 'X' catch-all; it was silently encoded as hydrogen. Single shared dataset_creation/atom_features.py replaces 4 copies; dead stale copy removed from GNN-DTI/utils.py. - De-duplicate the GCN (was copied across 5 files) into gcn_model.py; models read feature width from the data so an extended vocabulary just works. Honesty - README + Streamlit demo relabel the 0.95 as an in-sample sanity check and correct the "edges = spatial proximity" claim (the deployed GCN uses the covalent adjacency; geometry is used only by the GNN-DTI variant). Tests - New: structure-level split, imbalance metrics, focal loss, atom-feature Mg fix, GCN forward + shipped-checkpoint load (guards the demo), width-agnostic GNN-DTI. 24 passing; ruff + compileall clean. Note: the honest held-out number still requires regenerating the full corpus (only 1fuf.pkl ships) and running train_gnn.py, which now prints a HELD-OUT line.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c010e7c9b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| out = None | ||
| groups = [g.pdb for g in data_list] | ||
| train_idx, valid_idx, test_idx = group_train_val_test_split(groups, (0.8, 0.1, 0.1), seed=SEED) |
There was a problem hiding this comment.
When the saved-tensor corpus is built from the bundled nonredundant RNA list, it includes 4RUM, but this split is computed over the full data_list and only later loads pd4rum_graphs.pt as HELD-OUT. That lets 4RUM graphs land in train/valid/test while the same structure is reported as held out, so the advertised leakage-free HELD-OUT metric is contaminated; filter the 4RUM group out before calling group_train_val_test_split.
Useful? React with 👍 / 👎.
|
|
||
| out = None | ||
| groups = [g.pdb for g in data_list] | ||
| train_idx, valid_idx, test_idx = group_train_val_test_split(groups, (0.8, 0.1, 0.1), seed=SEED) |
There was a problem hiding this comment.
This variant has the same leakage path: if saved_tensors is generated from the nonredundant corpus containing 4RUM, the full data_list is grouped and split here, then pd4rum_graphs.pt is evaluated as HELD-OUT afterward. Any 4RUM tensors selected into train/valid/test make the held-out result in-sample, so exclude the held-out PDB before this split.
Useful? React with 👍 / 👎.
1 participant
Why
The headline ROC-AUC ≈ 0.95 on 1FUF was in-sample: 1FUF is in the training list (
data/nonredundantRNA.txt) and the splits shuffled candidate sites, so near-identical grid points from one structure leaked across train/val/test. This PR corrects the methodology, makes the claims honest, removes ~5× model duplication, and adds tests.Core fixes
rna_splits.py) — group by PDB id, never by site; hold whole structures out of the corpus (HELD_OUT_PDBS). Wired intotrain_gnn{,1,2}.pyand the GNN-DTI trainer.pos_weightinBCEWithLogitsLoss; report PR-AUC + enrichment + precision/recall@Youden alongside ROC-AUC (rna_metrics.py).rna_seed.py).(1 − p_t)^γper example (was reduced to a scalar first, collapsing it to a scaled BCE).Mgis now an explicit symbol with anXcatch-all (it was silently encoded as hydrogen). One shareddataset_creation/atom_features.pyreplaces 4 copies; dead stale copy removed fromGNN-DTI/utils.py.gcn_model.py; models read feature width from the data.Honesty
A1); geometry (A2) is used only by the GNN-DTI variant.Test plan
ruff check .✅ ·python -m compileall -q .✅ ·pytest -q✅ 24 passed (was 3)best_model.pthstill loads (guards the demo), width-agnostic GNN-DTI.predfrommodel.pyon real 1FUF → reproducesROC-AUC=0.951 PR-AUC=0.014 top 4.9%, now labelled in-sample.Only
1fuf.pklships, not the ~397-structure corpus, so the honest held-out number can't be produced here. To finish: regenerate pickles (dataset_creation/gnn_rna.py) on the full PDB set and runtrain_gnn.py, which now prints aHELD-OUT:line. Expect it to be lower than 0.95 — that's the point.