Add ss_opm - #16
Conversation
VladimirShitov
left a comment
VladimirShitov
left a comment
There was a problem hiding this comment.
In general, looks good as the first approximation. I would suggest a couple of improvements:
- It is concerning that not all data types are provided to the model (e.g., cell type ratios). Ideally, these code parts should be generalised to be applicable to any datasets (but perhaps this is more of a task for the upstream model code).
- Additionally, some assumptions about the batch format are made, e.g., that they contain day information in the format
d1,d2, etc. This should either be generalised or clearly documented if somebody wants to plug in a custom dataset.
Let me know if you think these are not the problems. I will double-check what the preprocessing in the original code looks like and comment again.
| def build_metadata(adata, task_type): | ||
| """Build a metadata DataFrame compatible with ss_opm from an H5AD AnnData. | ||
|
|
||
| Mirrors the function in the train script; only used for the input preprocessing |
There was a problem hiding this comment.
Can we move this function somewhere and generalise? The code is largely repeated between scripts
There was a problem hiding this comment.
We can also move it to the ss_opm package
| obs['donor'] = 0 | ||
| obs['technology'] = 'unknown' | ||
|
|
||
| if task_type == 'cite': |
There was a problem hiding this comment.
Can we generalize this, e.g., by providing cell_type_obs_col argument to the function, if cell types are available? The code of the original model might hardcode the particular cell type names, though... Ideally, it should be generalised as well
| obs = pd.DataFrame(index=adata.obs_names) | ||
|
|
||
| obs['batch'] = adata.obs['batch'].values | ||
| obs['day'] = adata.obs['batch'].str.extract(r'd(\d+)', expand=False).astype(float).fillna(0).values |
There was a problem hiding this comment.
This is also a bit dataset specific :/ But might be fine, we can make it a data specification. However, if the batch col doesn't have information about the day in this format, there will be an error
| pre_post_process = pickle.load(f) | ||
|
|
||
| model = EncoderDecoder(params=None) | ||
| # PyTorch >=2.6 defaults weights_only=True, which blocks custom classes. |
There was a problem hiding this comment.
Can we simply fix a particular version of torch?
| set_seed(SEED) | ||
|
|
||
|
|
||
| def build_metadata(adata, task_type): |
There was a problem hiding this comment.
As mentioned above, this is largely overlapping with the function in the prediction script
# Conflicts: # src/workflows/run_benchmark/config.vsh.yaml # src/workflows/run_benchmark/main.nf
* Move `build_metadata()` and `to_sparse_csr()` into `ss_opm_common.py`, shared by both scripts instead of copied between them * Add `--cell_type_col`, so the `cell_ratio_*` features come from real cell types when a dataset has them. This task's file format does not carry cell types, so the default stays uniform * Add `--day_pattern` and document that a batch label which does not match yields day 0 rather than an error * Add `--n_epochs` and `--burnin_length_epoch`, capped during `viash test` with `info.test_default` * Report `ss_opm` as the method id rather than `ss_opm_predict` * Give `ss_opm_predict` a `test_setup` and build its model fixture in `test_resources.sh`
2 participants
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:
Proposed changes are described in the CHANGELOG.md
CI Tests succeed and look good!