Skip to content

Adding new method: scMerge2 - #63

Merged
rcannood merged 13 commits into
openproblems-bio:mainfrom
seohyonkim:feature/scmerge
Aug 21, 2026
Merged

Adding new method: scMerge2#63
rcannood merged 13 commits into
openproblems-bio:mainfrom
seohyonkim:feature/scmerge

Conversation

@seohyonkim

Copy link
Copy Markdown
Contributor

Describe your changes

This PR is for a new method scMerge2.

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!

@seohyonkim
seohyonkim marked this pull request as draft June 4, 2025 20:47
@seohyonkim

Copy link
Copy Markdown
Contributor Author

@lazappi Hi! This is Seo :)
The previously-working version of this method is failing now, could you please see why? Is there anything new that has changed?
Thank you so much!

Comment thread src/methods/semisupervised_scmerge2/config.vsh.yaml Outdated
seohyonkim and others added 2 commits July 23, 2025 17:33
Co-authored-by: Luke Zappia <lazappi@users.noreply.github.com>
@rcannood
rcannood requested a review from mumichae August 8, 2025 14:33
@rcannood

rcannood commented Aug 8, 2025

Copy link
Copy Markdown
Member

@mumichae Could you take a look at this PR?

@mumichae
mumichae marked this pull request as ready for review August 28, 2025 10:56
Comment thread src/methods/semisupervised_scmerge2/script.R Outdated
Comment thread src/methods/semisupervised_scmerge2/script.R Outdated
Comment thread src/methods/semisupervised_scmerge2/script.R Outdated
Comment thread src/methods/semisupervised_scmerge2/script.R Outdated
Comment thread src/methods/semisupervised_scmerge2/config.vsh.yaml Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
@seohyonkim

Copy link
Copy Markdown
Contributor Author

@mumichae I fixed up the code with the help of your feedback!
Here are some key points to help you review the code quicker (and also to help myself):

  • scMerge2 takes and returns the matrix of gene x cell, not cell x gene, so I transpose the matrix in the beginning and before storing it to the output
  • top_n for the SEG selection is set for 1000 by default, but for small test data, I did min(top_n, nrow(seg_df)) for safety
  • I did couple of lines such as rownames(counts) <- as.character(adata$var_names) since I'm scared of AnnData conversion dropping them
  • newY is the return format(?) of scMerge2

Let me know if there are any thing else that can be better :)

@mumichae mumichae left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already looking a lot better!
There are still some computational bottlenecks that are worth solving (given that this methods uses the complete count matrix and densifying is expensive.

Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
Comment thread src/methods/semisupervised_scmerge2/script.R Outdated
Comment thread src/methods/semisupervised_scmerge2/script.R Outdated
Comment thread src/methods/unsupervised_scmerge2/script.R Outdated
@rcannood

Copy link
Copy Markdown
Member

@seohyonkim Would you be able to take a look at @mumichae's comments?

@rcannood

rcannood commented Aug 21, 2026

Copy link
Copy Markdown
Member

I pushed a few changes to the branch, after merging main into this PR.

  • preferred_normalization: log_cpm. This task's datasets are log_cp10k, l1_sqrt and sqrt_cp10k, so the runEach filter never matched and the method would have been silently skipped on every dataset.
  • method_types: [feature] with an X_emb-only output. transform asserts corrected_counts is present for a feature method, so every task would have died in process_integration.Now [embedding], which is what the script actually produces.
  • One component instead of two, as @mumichae suggested.
  • --n_control_genes and --n_dim as parameters
  • BiocParallel::MulticoreParam(meta$cpus) passed to both calls, which were running serially
  • added to run_benchmark/main.nf and its dependencies:, without which it builds and tests but never runs
  • labels bumped to [hightime, highmem, midcpu]; the SEG fit is per gene across all cells and the corrected matrix is dense

@rcannood
rcannood merged commit 75b79da into openproblems-bio:main Aug 21, 2026
3 of 4 checks passed
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.

4 participants