Add cluster metrics viz - #11372
Conversation
|
Um, you need to rebase these changes on the current master, as there was a bulk update (and you have a massive conflict ;) ). |
c038148 to
9e0f196
Compare
|
Hmm, this is an entirely custom set of modules? |
|
Thanks @SPPearce, that makes sense. I’ve moved both custom Python-based modules under Both module tests now pass locally for |
SPPearce
left a comment
There was a problem hiding this comment.
I think it'd need to be custom/clustermetrics and custom/clustervisualiation (no underscores allowed).
You'll need to use template directly, rather than referring to ${projectDir}/modules/nf-core/custom/cluster_metrics/templates/cluster_metrics.py.
|
Thanks @SPPearce — I renamed the custom modules to remove underscores and updated both modules to use |
- cluster_metrics: computes clustering quality metrics + k-sweep - cluster_viz: generates PCA, UMAP and t-SNE plots colored by cluster - Both use conda environment.yml - Full nf-test coverage
462fff8 to
e4877e4
Compare
SPPearce
left a comment
There was a problem hiding this comment.
You can't use moduleDir at all in the main.nf, it won't work on cloud systems as the python script won't be there.
You need to use a template, like these modules.
There was a problem hiding this comment.
You could make the docker/singularity containers via Seqera Containers rather than needing a custom image in the nf-core quay.io.
pinin4fjords
left a comment
There was a problem hiding this comment.
Hi @dbaku42, thanks for the contribution. AI-assisted review (Claude, on behalf of @pinin4fjords). Some overlap with @SPPearce's earlier feedback. Beginner-friendly notes plus suggestion blocks you can apply directly.
The biggest fix is the templates/ mechanism. Right now your script: block runs python3 ${moduleDir}/templates/cluster_metrics.py, which is just running a normal Python script that happens to live in templates/. No templating is happening. This works on your laptop because ${moduleDir} resolves to a local path, but it breaks on cloud/HPC executors because that folder is not staged into the work directory the task runs in.
Nextflow's template directive does two things:
- Stages the file into the task work directory automatically (so it works on any executor).
- Runs the file through Groovy GString interpolation before staging, so
${features},${task.ext.prefix}, etc. get substituted into the Python source. This replacesargparseentirely.
Because the file goes through Groovy interpolation, a few Python source patterns need adjusting (\n -> \\n, r"\s+" -> r"\\s+", etc., I verified this locally). I have flagged the specific spots inline. See modules/nf-core/custom/tx2gene/templates/tx2gene.py for the canonical shape.
Two scope questions for discussion:
- The
*_selected.json"best k" output bakes a decision into the module, and the sweep refits KMeans regardless of what method produced the input labels. Could the module just emit the k-sweep table and let the consuming pipeline pick? cluster_vizdoes PCA + UMAP + t-SNE + plotting + TSV export, and the PCA piece is essentially a re-plot of upstream PLINK eigenvec output. Consider dropping PCA plotting from this module and tightening to the embedding methods that genuinely belong together.
The clustervisualiation directory name looks like a typo for clustervisualization; worth fixing while you're renaming things.
Co-authored-by: Jonathan Manning <pininforthefjords@gmail.com>
Co-authored-by: Jonathan Manning <pininforthefjords@gmail.com>
Co-authored-by: Jonathan Manning <pininforthefjords@gmail.com>
Co-authored-by: Jonathan Manning <pininforthefjords@gmail.com>
…iz.py Co-authored-by: Jonathan Manning <pininforthefjords@gmail.com>
…iz.py Co-authored-by: Jonathan Manning <pininforthefjords@gmail.com>
Added copyright and licensing information to the file.
Added copyright and licensing information to the top of the file.
SPPearce
left a comment
There was a problem hiding this comment.
You seem to still have pcaclustering as well as plink2pcaclustering.
Actually, I'm wondering if this should just go into plink2/pcaclustering
Replace the PLINK2 .eigenvec parser with a plain pandas read of a TSV that has a `sample_id` column plus numeric feature columns - the same input contract `custom/clustermetrics` already uses. PLINK2 eigenvec output is one supported source (drop FID, rename IID -> sample_id), not the only one. Also removes the duplicate `plink2pcaclustering/` directory left behind by an in-flight rename. clusters.csv / clustering_info.json md5s are byte-identical to those recorded in the existing snapshot; the .snap file is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@SPPearce @dbaku42 - while looking at the Verified locally that the existing snapshot stays byte-identical (proof in the PR body): dbaku42#1 Targeted at @dbaku42's branch as a proposal, not a request - if you'd rather go the other way and nest under AI-assisted (Claude, on behalf of @pinin4fjords). |
…c-input Make custom/pcaclustering accept generic sample_id TSV
|
Thanks @pinin4fjords! Keeping it general is a better approach. I've already merged your proposal. |
OK, lemme fix what's broken |
… fixture The earlier generic-input refactor (PR nf-core#11372 / #1) pointed at `popgen/clustering/test.tsv`, but the actual fixture in nf-core/test-datasets (added in nf-core/test-datasets#2051) is named `test_features.tsv`. CI was failing with "No such file or directory" on all three pcaclustering tests as a result. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed |
Use the first column of the features TSV as sample IDs regardless of its header name, and treat all remaining columns as numeric features. The required column name was a hidden assumption about input shape - this relaxes it without adding inputs, so callers can feed in e.g. a reformatted PLINK eigenvec (with `IID` or any other ID header) directly, as long as the ID column is first. Output schema (clusters.csv emits `sample_id,cluster`) is unchanged, so downstream consumers (clustermetrics, clustervisualization) see the same contract. Verified clusters.csv / clustering_info.json md5s remain byte-identical to the snapshot against the existing test fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SPPearce
left a comment
There was a problem hiding this comment.
That seems much better.
Just to check, does it work if the first column (the sample id's) are all numeric?
Pin the first column to `dtype=str` at read time so pandas doesn't type-infer it as int and strip leading zeros (e.g. "0001" -> 1). Same output for the existing fixture (which has string IDs already); snapshot md5s unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@SPPearce: computer says yes. Plain integer IDs work fine, but I pushed |
|
Thanks @pinin4fjords for your help! Regarding the test.tsv file, I already opened a PR nf-core/test-datasets#2073 |
|
Well, tests are passing now. FYI I'm about to go on leave. You have my approval, I'll let @SPPearce give his final approval when he's ready. |
Description
This PR adds two new modules:
cluster_metrics: Computes standard clustering quality metrics (Silhouette score, Calinski-Harabasz index, Davies-Bouldin index) and performs a k-sweep analysis.cluster_viz: Generates 2D visualizations (PCA, UMAP, t-SNE) colored by cluster label and exports the coordinates as TSV files.Both modules are designed to work together with the existing
clusteringmodule (which performs KMeans/DBSCAN on PLINK2 PCA results).Features
condaviaenvironment.ymlnf-test(normal + stub tests)Author
Related modules
clustering)This completes the core components of the
snpclusteringsubworkflow.