Skip to content

Add ERA ComBat BBKNN method. - #84

Open
cmclean wants to merge 7 commits into
openproblems-bio:mainfrom
cmclean:feature/task_batch_integration/add-bbknn_ts-method
Open

Add ERA ComBat BBKNN method.#84
cmclean wants to merge 7 commits into
openproblems-bio:mainfrom
cmclean:feature/task_batch_integration/add-bbknn_ts-method

Conversation

@cmclean

@cmclean cmclean commented Sep 13, 2025

Copy link
Copy Markdown

Describe your changes

This PR adds the top-performing "BBKNN (TS)" method from our recent preprint [1]. I have verified that tests pass, and running the run_full_local.sh script on my own machine successfully recapitulates the numbers published in the preprint.

[1] Aygun et al, An AI system to help scientists write expert-level empirical software, arXiv:2509.06503 (2025), https://arxiv.org/abs/2509.06503 .

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!

@cmclean

cmclean commented Sep 15, 2025

Copy link
Copy Markdown
Author

I have a couple questions about the submission and testing:

  1. Should my new 'bbknn_ts' method also be registered into the following two places?
  • methods = [...] block of src/workflows/run_benchmark/main.nf (lines 10-46)
  • dependencies: block of src/workflows/run_benchmark/config.vsh.yaml (lines 79-125)
  1. (For my own edification) When I ran run_full_local.sh, the method completed successfully on all datasets and metrics except computing kbet on the hypomap dataset, where it OOMed. That was expected since it OOMed there on our internal evaluation harness, and many methods fail on that particular (dataset, method). To get the final score_uns.yaml file though I had to rerun while dropping the kbet metric (for all datasets). Is there a way to skip just a specified set of (dataset, method) jobs or ignore errors and finish the workflow just with what was computed properly? As is I was not able to validate identical kbet performance metrics, though given the identical results for all other metrics on all datasets am confident in its reproducibility.

Thanks!

@lazappi

lazappi commented Sep 17, 2025

Copy link
Copy Markdown
Member
1. Should my new 'bbknn_ts' method also be registered into the following two places?

* `methods = [...]` block of src/workflows/run_benchmark/main.nf (lines 10-46)
* `dependencies:` block of src/workflows/run_benchmark/config.vsh.yaml (lines 79-125)

Yes, it needs to be added there to be included in the workflow. It sounds like you ran the workflow already but I'm not sure that it would include the new method without doing this.

2. (For my own edification) When I ran `run_full_local.sh`, the method completed successfully on all datasets and metrics except computing `kbet` on the `hypomap` dataset, where it OOMed. That was expected since it OOMed there on our internal evaluation harness, and many methods fail on that particular (dataset, method). To get the final `score_uns.yaml` file though I had to rerun while dropping the `kbet` metric (for all datasets). Is there a way to skip just a specified set of (dataset, method) jobs or ignore errors and finish the workflow just with what was computed properly? As is I was not able to validate identical `kbet` performance metrics, though given the identical results for all other metrics on all datasets am confident in its reproducibility.

When we do the full benchmark run on the cloud any failed metrics are ignored (or more accurately given a score of zero). We don't usually do the full runs locally so there might be some differences in the settings that causes it to not produce an output. Generally we wouldn't want to disable a metric just for a specific dataset/method.

@lazappi
lazappi requested a review from mumichae September 17, 2025 12:55
@cmclean

cmclean commented Sep 19, 2025

Copy link
Copy Markdown
Author

Thank you for the responses. I noticed that the drvi method addition (#61) did not add to the src/workflows/run_benchmark/ files so didn't put bbknn_ts there but will in a follow-up commit in this PR.

Regarding my other question, this was ultimately just a vanilla NextFlow question, I've now figured out how to pipe errorStrategy 'ignore' into the right place. Thank you!

@cmclean

cmclean commented Sep 22, 2025

Copy link
Copy Markdown
Author

Okay this PR is ready for review. @mumichae please note that src/methods/bbknn_ts/script.py lines 18-226 does not require detailed review comments, this is an entirely LLM-generated function that we do not want to modify.

Comment thread src/methods/bbknn_ts/config.vsh.yaml Outdated
Comment thread src/methods/bbknn_ts/config.vsh.yaml Outdated
Comment thread src/methods/bbknn_ts/script.py
Comment thread CHANGELOG.md Outdated
@cmclean
cmclean requested a review from mumichae September 29, 2025 15:13
@cmclean

cmclean commented Oct 14, 2025

Copy link
Copy Markdown
Author

Friendly ping -- do you need anything else prior to re-reviewing this PR @mumichae? Thank you for your help.

Comment thread src/methods/bbknn_ts/config.vsh.yaml Outdated
Comment thread src/workflows/run_benchmark/config.vsh.yaml Outdated
Comment thread src/workflows/run_benchmark/main.nf Outdated
@mumichae

Copy link
Copy Markdown
Collaborator

Hi @cmclean, apologies for my late reply, I've been swamped with events the last couple weeks. I had a look at the prompt and the code in more detail, and found that it suggest a rather unconventional workflow, since you're running 2 integration benchmarks back-to-back. But since the point of openproblems is to benchmark novel approaches, so it's definitely interesting to include this approach.

I added some clarification on the relationship between preprocessing and integration here, but ultimately decided to keep the LLM approach as its own end-to-end workflow, where preprocessing steps aren't tunable

@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.

Review on the output type of the method to be consistent with the evaluation workflow (i.e. the correct integrated representation gets evaluated)

Comment thread src/methods/bbknn_ts/config.vsh.yaml Outdated
Comment thread src/methods/bbknn_ts/config.vsh.yaml Outdated
@rcannood

Copy link
Copy Markdown
Member

Hi @cmclean! Would you be able to address @mumichae's comments?

@cmclean

cmclean commented Aug 13, 2026

Copy link
Copy Markdown
Author

As I understand it the above comments suggest to split the two features out to be evaluated independently. But that seems at odds with both the description of the v2.0.0 version of the leaderboard ("The batch integrated output can be a feature matrix, a low dimensional embedding and/or a neighbourhood graph") and the implementations provided by some existing methods that provide multiple outputs (e.g. scalex, scanorama)?

I wrote questions in more detail at https://docs.google.com/document/d/1i2PqZjEVU8O544VbPQ9Bxh6zJai52LxK8D0Q0YXr4hY/edit?resourcekey=0-4x66cW9213nDd13ts0JhhQ&tab=t.0

@rcannood

rcannood commented Aug 21, 2026

Copy link
Copy Markdown
Member

Hi @cmclean!

The "and/or" in the leaderboard description does apply here. No need to split anything in this case; a single component can and should emit both an embedding and a graph, it's part of the same method. I'll look into why I approved the scanorama split pr.

Since your code sets uns['neighbors'], transform will not recompute the kNN graph from the embedding.

Proposed changes:

Config:

  • method_types: [embedding] -> [embedding, graph].
  • preferred_normalization: log_cp10k -> counts. The script reads layers/counts and normalises itself.
  • I'd leave out feature / corrected_counts: ComBat runs here after sc.pp.scale(), so the matrix is z-scored and clipped, and hvg_overlap calls scib.hvg_overlap() -> highly_variable_genes(flavor='cell_ranger') on it. That would error rather than add a score.
  • The reference probably needs to be updated to https://www.nature.com/articles/s41586-026-10658-6.

Script:

  • The three --arguments are currently dead -- the hardcoded config dict wins. Passing par through at the call site fixes it: eliminate_batch_effect_fn(input_adata, config={k: par[k] for k in ('n_pca_components', 'n_neighbors_per_batch', 'total_k_neighbors')}).
  • Build a minimal output AnnData with just X_emb, obsp, uns instead of writing the mutated input -- as is, the output carries a dense scaled X plus X_pca and the full obs/var.
  • output.uns['method_id'] = 'bbknn_ts' -> meta['name']

What do you think about a name like era_combat_bbknn?

cmclean and others added 2 commits August 21, 2026 18:35
1. Renamed to `era_combat_bbknn`.
2. Fixed the human-written parts of script.py to save a lightweight
   output and use the VIASH-injected config values.
3. Updated `method_types` to reflect it is both an embedding and graph.
4. Updated paper reference.
@cmclean

cmclean commented Aug 21, 2026

Copy link
Copy Markdown
Author

Thank you for the additional review @rcannood! I've submitted a new commit that addresses your proposed changes and all of @mumichae comments that are consistent with this being an embedding+graph method.

I made sure I can execute the updated script.py locally but have not run it through the full pipeline; as long as only obsm['X_emb'], obsp, and uns are needed then it should be fine.

@rcannood rcannood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the quick response! Almost lgtm, a few last remarks about the config.

I made sure I can execute the updated script.py locally but have not run it through the full pipeline; as long as only obsm['X_emb'], obsp, and uns are needed then it should be fine.

If the CI already passes, that's good enough. Any issues when the method is ran as part of the wf will get flagged in the issues later on ^^

With these last changes I'm happy to merge the PR.

label: ERA ComBat BBKNN
summary: "A combination of ComBat and BBKNN discovered and implemented by Gemini."
description: |
"The ERA ComBat BBKNN solution (named "BBKNN (TS)" in the associated publication) applies standard scRNA-seq preprocessing steps, including total count normalization, log-transformation, and scaling of gene expression data. Batch effect correction is performed using scanpy.pp.combat directly on the gene expression matrix (before dimensionality reduction). Dimensionality reduction is then applied using PCA on the ComBat-corrected data, and this PCA embedding (adata.obsm['X_pca']) is designated as the integrated embedding (adata.obsm['X_emb']). A custom batch-aware nearest neighbors graph is constructed based on this integrated embedding; for each cell, neighbors are independently identified within its own batch and other batches, up to n_neighbors_per_batch. These candidate neighbors are merged, keeping the minimum distance for duplicate entries, and the top total_k_neighbors are selected for each cell. Finally, a symmetric sparse distance matrix and a binary connectivities matrix are generated to represent the integrated neighborhood graph. This code was entirely written by the AI system described in the associated publication."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're using |, the extra quotes are redundant

Suggested change
"The ERA ComBat BBKNN solution (named "BBKNN (TS)" in the associated publication) applies standard scRNA-seq preprocessing steps, including total count normalization, log-transformation, and scaling of gene expression data. Batch effect correction is performed using scanpy.pp.combat directly on the gene expression matrix (before dimensionality reduction). Dimensionality reduction is then applied using PCA on the ComBat-corrected data, and this PCA embedding (adata.obsm['X_pca']) is designated as the integrated embedding (adata.obsm['X_emb']). A custom batch-aware nearest neighbors graph is constructed based on this integrated embedding; for each cell, neighbors are independently identified within its own batch and other batches, up to n_neighbors_per_batch. These candidate neighbors are merged, keeping the minimum distance for duplicate entries, and the top total_k_neighbors are selected for each cell. Finally, a symmetric sparse distance matrix and a binary connectivities matrix are generated to represent the integrated neighborhood graph. This code was entirely written by the AI system described in the associated publication."
The ERA ComBat BBKNN solution (named "BBKNN (TS)" in the associated publication) applies standard scRNA-seq preprocessing steps, including total count normalization, log-transformation, and scaling of gene expression data. Batch effect correction is performed using scanpy.pp.combat directly on the gene expression matrix (before dimensionality reduction). Dimensionality reduction is then applied using PCA on the ComBat-corrected data, and this PCA embedding (adata.obsm['X_pca']) is designated as the integrated embedding (adata.obsm['X_emb']). A custom batch-aware nearest neighbors graph is constructed based on this integrated embedding; for each cell, neighbors are independently identified within its own batch and other batches, up to n_neighbors_per_batch. These candidate neighbors are merged, keeping the minimum distance for duplicate entries, and the top total_k_neighbors are selected for each cell. Finally, a symmetric sparse distance matrix and a binary connectivities matrix are generated to represent the integrated neighborhood graph. This code was entirely written by the AI system described in the associated publication.

Comment on lines +9 to +17
bibtex: |
@article{GoogleScienceAI2026,
title={An {AI} system to help scientists write expert-level empirical software},
author={Ayg{\"u}n, Eser and Belyaeva, Anastasiya and Comanici, Gheorghe and Coram, Marc and Cui, Hao and Garrison, Jake and Johnston, Renee and Kast, Anton and McLean, Cory Y and Norgaard, Peter and others},
journal={Nature},
volume={654},
pages={909-–916},
year={2026},
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the publication has a doi, it's easier to just use the doi

Suggested change
bibtex: |
@article{GoogleScienceAI2026,
title={An {AI} system to help scientists write expert-level empirical software},
author={Ayg{\"u}n, Eser and Belyaeva, Anastasiya and Comanici, Gheorghe and Coram, Marc and Cui, Hao and Garrison, Jake and Johnston, Renee and Kast, Anton and McLean, Cory Y and Norgaard, Peter and others},
journal={Nature},
volume={654},
pages={909-–916},
year={2026},
}
# Aygün, E., Belyaeva, A., Comanici, G. et al.
# An AI system to help scientists write expert-level empirical software.
# Nature 654, 909-916 (2026). https://doi.org/10.1038/s41586-026-10658-6
doi: 10.1038/s41586-026-10658-6


engines:
- type: docker
image: openproblems/base_python:1.0.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
image: openproblems/base_python:1.0.0
image: openproblems/base_python:1

Use the :1 tag to get updates from time to time

@rcannood rcannood changed the title Add BBKNN (TS) method. Add ERA ComBat BBKNN method. Aug 21, 2026
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