feat(custom/clustermetrics, custom/clustervisualization): accept PLIN… - #12906
Conversation
…K2 eigenvec, add k-distance plot, set n_jobs=1
erikrikarddaniel
left a comment
erikrikarddaniel
left a comment
There was a problem hiding this comment.
Claude didn't find complete test coverage: "Missing test coverage — the new IID/FID/SID PLINK2-header parsing branch in load_features isn't exercised by any test in either module; both still only feed the plain-TSV fixture, and even custom/pcaclustering's existing "eigenvec" test turns out to reuse that same TSV rather than a real .eigenvec-shaped file."
Two more comments in the code.
Not blocking, but worth thinking about.
| if "IID" in df.columns: | ||
| df = df.rename(columns={"IID": "sample_id"}) | ||
| elif "sample_id" not in df.columns: | ||
| df = df.rename(columns={df.columns[0]: "sample_id"}) |
There was a problem hiding this comment.
Worth double-checking: when neither IID nor sample_id is present, this now silently renames whichever column happens to be first to sample_id, instead of the previous behavior of raising the ValueError below (line 48) immediately. A genuinely malformed/mislabeled input (columns in the wrong order, or an ID column named something other than sample_id/IID) would now be used as-is rather than failing with the old, clear "features file must have a 'sample_id' column" message — any mismatch would instead surface later as a confusing empty join or a garbage plot. Is the silent fallback intentional (e.g. to support a headerless first-column-is-ID convention), or should this elif just keep raising, with only the two known PLINK2 header shapes special-cased above it?
| if "IID" in df.columns: | ||
| df = df.rename(columns={"IID": "sample_id"}) | ||
| elif "sample_id" not in df.columns: | ||
| df = df.rename(columns={df.columns[0]: "sample_id"}) |
There was a problem hiding this comment.
Same fallback question as in clustermetrics/templates/cluster_metrics.py — see that comment.
… on missing sample_id column Remove the silent first-column fallback in load_features; only IID is special-cased for PLINK2 headers, everything else without sample_id still raises the explicit ValueError. Add eigenvec-based test coverage using the existing test.eigenvec fixture in test-datasets, exercising the PLINK2 header-parsing branch.
|
Good catch — that silent fallback wasn't intentional. Removed the elif branch in both cluster_metrics.py and cluster_viz.py; now only IID is special-cased, and anything without sample_id/IID raises the original ValueError again instead of silently renaming the first column. Also added a test in each module (clustermetrics - features from eigenvec and clustervisualization - features from eigenvec) using test.eigenvec from test-datasets, which exercises the actual PLINK2 header-parsing branch — turns out it was already sitting unused in the popgen/clustering folder. Snapshots updated accordingly. |
|
thanks @erikrikarddaniel, |
You have my approval, just go ahead and merge! |
2 participants
This PR updates
load_featuresin both modules so they accept the sameinputs as
custom/pcaclustering:sample_idcolumn (unchanged).eigenvec(#FID IID PC1 ...or#IID PC1 ...);FID/SIDare dropped and
IIDis used assample_idAlso:
custom/clustermetrics: k-distance plot (*.k_distance.png) to helpchoose DBSCAN
eps. Optionalext.args:--k-neighbors,--eps.custom/clustervisualization:n_jobs=1on UMAP and t-SNE (with theexisting
random_state=42) to stabilize embeddings across runners.main.nfinput signatures are unchanged (path(features)). Docs inmeta.ymlnow list*.{tsv,txt,eigenvec}.Related
GAWK_EIGENVEC_TO_TSVafter this)