Add Seurat components by rcannood · Pull Request #77 · openproblems-bio/task_batch_integration

rcannood · 2025-08-20T14:53:19Z

Describe your changes

Checklist before requesting a review

I have performed a self-review of my code
Check the correct box. Does this PR contain:
- Breaking changes
- New functionality
- Major changes
- Minor changes
- Bug fixes
Proposed changes are described in the CHANGELOG.md
CI Tests succeed and look good!

lazappi · 2025-08-26T06:11:54Z

src/methods/seurat_cca/script.R

+  dense_temp <- as.matrix(norm_data)
+  norm_data <- as(dense_temp, "dgCMatrix")


Could this be one line to avoid making an extra copy?

Thanks for the review!

Can do!

Is intermediate dinsification strictly necessary? I'm worried that could cause a computational bottleneck for larger data

src/methods/seurat_cca/script.R

lazappi · 2025-08-26T06:19:09Z

src/methods/seurat_cca/script.R

+cat("Generate UMAP embedding\n")
+integrated <- RunUMAP(
+  integrated,
+  reduction = "pca",
+  dims = seq_len(par$dims),
+  verbose = FALSE
+)
+
+cat("Extract embedding\n")
+embedding <- Embeddings(integrated, reduction = "umap")


Why generate and store a UMAP instead of the integrated embedding?

I agree with that, shouldn't it instead be:

Suggested change

cat("Generate UMAP embedding\n")

integrated <- RunUMAP(

integrated,

reduction = "pca",

dims = seq_len(par$dims),

verbose = FALSE

)

cat("Extract embedding\n")

embedding <- Embeddings(integrated, reduction = "umap")

cat("Extract embedding\n")

embedding <- Embeddings(integrated, reduction = "pca")

Using UMAP for anything other than visualisation isn't common practice (and also not the default way of running the method)

I think the integration might be stored as something like "integrated" but you would need to check the function calls.

I would suggest following this vignette https://satijalab.org/seurat/articles/seurat5_integration.

lazappi · 2025-08-26T06:21:26Z

src/methods/seurat_cca/script.R

+anchors <- FindIntegrationAnchors(
+  object.list = seurat_list,
+  anchor.features = hvg_genes,
+  dims = seq_len(par$dims),
+  k.anchor = par$k_anchor,
+  k.filter = par$k_filter,
+  k.score = par$k_score,
+  verbose = FALSE
+)
+
+# Integrate the data
+integrated <- IntegrateData(
+  anchorset = anchors,
+  dims = seq_len(par$dims),
+  verbose = FALSE
+)


{Seurat} v5 has a new IntegrateLayers function which we should probably use instead https://satijalab.org/seurat/articles/seurat5_integration#perform-streamlined-one-line-integrative-analysis

That's a good point. If the models are still the same, we should use the latest implementation for it.

src/methods/seurat_cca/script.R

mumichae · 2025-08-29T10:09:54Z

src/methods/seurat_rpca/script.R

+counts_data <- t(adata$layers[["counts"]])
+norm_data <- t(adata$layers[["normalized"]])


are both matrices needed?

Feature selection should ideally be standardised across methods

mumichae · 2025-08-29T10:15:42Z

src/methods/seurat_cca/script.R

+cat("Generate UMAP embedding\n")
+integrated <- RunUMAP(
+  integrated,
+  reduction = "pca",
+  dims = seq_len(par$dims),
+  verbose = FALSE
+)
+
+cat("Extract embedding\n")
+embedding <- Embeddings(integrated, reduction = "umap")


I agree with that, shouldn't it instead be:

Suggested change

cat("Generate UMAP embedding\n")

integrated <- RunUMAP(

integrated,

reduction = "pca",

dims = seq_len(par$dims),

verbose = FALSE

)

cat("Extract embedding\n")

embedding <- Embeddings(integrated, reduction = "umap")

cat("Extract embedding\n")

embedding <- Embeddings(integrated, reduction = "pca")

Using UMAP for anything other than visualisation isn't common practice (and also not the default way of running the method)

mumichae · 2025-08-29T10:16:18Z

src/methods/seurat_rpca/script.R

+cat("Generate UMAP embedding\n")
+integrated <- RunUMAP(integrated, reduction = "pca", dims = seq_len(par$dims), verbose = FALSE)
+
+cat("Extract embedding\n")
+embedding <- Embeddings(integrated, reduction = "umap")


Same as comment in CCA

Suggested change

cat("Generate UMAP embedding\n")

integrated <- RunUMAP(integrated, reduction = "pca", dims = seq_len(par$dims), verbose = FALSE)

cat("Extract embedding\n")

embedding <- Embeddings(integrated, reduction = "umap")

cat("Extract embedding\n")

embedding <- Embeddings(integrated, reduction = "pca")

mumichae · 2025-09-25T09:42:06Z

@rcannood Could you consider updating to the Seurat v5 implementation? That would also resolve most of our reviews

rcannood added 2 commits August 20, 2025 16:52

add suerat components

89d36ac

update changelog

0a32705

rcannood requested a review from mumichae August 20, 2025 15:10

lazappi requested changes Aug 26, 2025

View reviewed changes

mumichae requested changes Aug 29, 2025

View reviewed changes

Merge branch 'main' into add-seurat-methods

d66e5cb

		dense_temp <- as.matrix(norm_data)
		norm_data <- as(dense_temp, "dgCMatrix")

		counts_data <- t(adata$layers[["counts"]])
		norm_data <- t(adata$layers[["normalized"]])

Conversation

rcannood commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Checklist before requesting a review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumichae commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rcannood commented Aug 20, 2025 •

edited

Loading