Skip to content

Add sciPENN method (GEX->ADT) - #72

Open
benjaminfreyuu wants to merge 2 commits into
openproblems-bio:mainfrom
benjaminfreyuu:feat/scipenn
Open

benjaminfreyuu wants to merge 2 commits into
openproblems-bio:mainfrom
benjaminfreyuu:feat/scipenn

Conversation

@benjaminfreyuu

Copy link
Copy Markdown
Contributor

Wraps sciPENN 0.9.6 (Lakkis et al. 2022) for CITE-seq protein prediction and registers it in run_benchmark. Unsupported directions raise.

Works around upstream bugs that previously made training look like an unbounded memory leak:

  • build_dir loops forever on absolute paths (os.path.split("/") never yields ""); replaced with os.makedirs.
  • float64 input crashes the float32 BatchNorm layers; cast to float32.

Feeds sciPENN the log_cp10k layers with its own normalize_total/log1p disabled, and maps its z-scored predictions back to log_cp10k using the training-protein mean/std, so outputs match the ground-truth space.

Also: arguments use default instead of example (they were None at runtime), add --seed, install requests for check_config, and stop listing run_and_check_output twice in test_resources.

Describe your changes

Checklist before requesting a review

  • [ x] I have performed a self-review of my code

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • [ x] New functionality
    • [x ] Major changes
    • Minor changes
    • Bug fixes
  • Proposed changes are described in the CHANGELOG.md

  • [x ] CI Tests succeed and look good!

Wraps sciPENN 0.9.6 (Lakkis et al. 2022) for CITE-seq protein prediction
and registers it in run_benchmark. Unsupported directions raise, like
babel and guanlab_dengkw_pm.

Works around upstream bugs that previously made training look like an
unbounded memory leak:
- build_dir loops forever on absolute paths (os.path.split("/") never
  yields ""); replaced with os.makedirs.
- float64 input crashes the float32 BatchNorm layers; cast to float32.

Feeds sciPENN the log_cp10k layers with its own normalize_total/log1p
disabled, and maps its z-scored predictions back to log_cp10k using the
training-protein mean/std, so outputs match the ground-truth space.

Also: arguments use default instead of example (they were None at
runtime), add --seed, install requests for check_config, and stop
listing run_and_check_output twice in test_resources.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@benjaminfreyuu

Copy link
Copy Markdown
Contributor Author

@VladimirShitov Please also review if time

@VladimirShitov VladimirShitov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general looks good, but please take a look at the comments. You mentioned that ther ewas a problem with this method previously, what was it and how is it solved now?

Comment thread src/methods/scipenn/config.vsh.yaml Outdated
type: integer
description: "Training batch size."
default: 128
- name: "--select_hvg"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it is not model-specific, there is no need: HVGs are part of input data

Comment thread src/methods/scipenn/script.py Outdated
'input_train_mod2': r'resources_test/task_predict_modality/openproblems_neurips2021/bmmc_cite/swap/train_mod2.h5ad',
'input_test_mod1': r'resources_test/task_predict_modality/openproblems_neurips2021/bmmc_cite/swap/test_mod1.h5ad',
'output': r'resources_test/task_predict_modality/openproblems_neurips2021/bmmc_cite/swap/prediction.h5ad',
'n_epochs': int(r'10000'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why such a weird format and not just 10000?

Comment thread src/methods/scipenn/script.py Outdated
# every non-empty batch in both train and test has at least 2 cells; otherwise fall
# back to global scaling (batch keys disabled).
def _batches_safe(*adatas):
for a in adatas:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Never use one letter variable names please

@VladimirShitov

Copy link
Copy Markdown
Contributor

Also please fix the CI failing

…, cleanup

- Fix CI: build on openproblems/base_pytorch_nvidia:1 with `pip install --no-deps
  sciPENN==0.9.6` instead of pinning torch==1.13.1 from the pytorch cu117 index.
  The exclusive `--index-url` broke build-dependency resolution (flit_core not on
  that index); the base image ships a working torch/scanpy/anndata stack plus the
  openproblems core helper the test harness needs. sciPENN runs on it (verified).
- Drop --select_hvg/--min_cells/--min_genes: these are data preprocessing, not
  model hyperparameters, and are not exposed by the other methods. Their values
  are hardcoded in the script so behaviour is unchanged.
- Replace the auto-generated VIASH START block with plain literals.
- Rename all one-letter variables.

viash test: 2/2 pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@benjaminfreyuu

Copy link
Copy Markdown
Contributor Author

Thanks @VladimirShitov — addressed all the comments (viash test 2/2):

  • CI failure: it was the torch install — torch==1.13.1 --index-url https://download.pytorch.org/whl/cu117 made pip resolve all packages (incl. the flit_core build backend) from the pytorch index, which doesn't host them. Switched to building on openproblems/base_pytorch_nvidia:1 with just pip install --no-deps sciPENN==0.9.6 (the base ships a working torch/scanpy/anndata stack + the openproblems core helper the test harness needs). sciPENN runs fine on it.
  • select_hvg: removed — and for the same reason (min_cells/min_genes are data preprocessing, not model hyperparameters, and aren't exposed by the other methods) I dropped those two as well. Their values are hardcoded in the script so behaviour is unchanged.
  • int(r'10000'): that was the auto-generated viash config inject block; replaced with a hand-written VIASH START using plain literals.
  • One-letter variables: renamed all of them.

On the "previous problem": the July diagnosis ("training memory leak, needs GPU + highmem") was wrong. The real causes, now fixed:

  • sciPENN 0.9.6's build_dir infinite-loops on absolute paths (os.path.split("/") never returns "", so it appends "/" forever until the process is OOM-killed) — that was the "memory leak". Fixed by monkeypatching build_dir with os.makedirs. Actual training is ~2 s / ~430 MB on the test data.
  • float64 input crashed sciPENN's float32 BatchNorm → the script casts inputs to float32.
  • The config used example: instead of default:, so args arrived as None at runtime.
  • sciPENN emits per-batch z-scored proteins; the script now feeds log_cp10k and inverts predictions with the training-protein mean/std, which dropped RMSE from 3.48 to 1.27.

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.

2 participants