Cytotrace kernel

[michalk8]

IMPORTANT: Please search among the Pull request before creating one.

Title

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• New feature (non-breaking change which adds functionality)

Description

How has this been tested?

No tests yet, we should take ones from 3f56472 and add some more.

Closes

closes #528

TODOs:

• determine what should be saved in .var/.obs (I'd remove 'num_exp_genes' from saving into adata.obs)
• determine what params to expose (I'd expose n_top_genes: int = 200 - more flexibility) + maybe add key_added: str = 'gcs' and write params to adata.uns as adata.uns[f'{key_added}_params'] = {'agg': 'mean', 'raw': True, ...}
• determine the best way to expose/not expose cyto_trace function (currently it's in the public API, because I refer to it from CytoTRACEKernels's init; one alternative would be to make a method of the kernel class (I don't think this is such a good idea).
• tests
• update docstrings

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (dev@e2c08da). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev     #527   +/-   ##
======================================
  Coverage       ?   88.49%           
======================================
  Files          ?       55           
  Lines          ?     7585           
  Branches       ?        0           
======================================
  Hits           ?     6712           
  Misses         ?      873           
  Partials       ?        0           

Flag	Coverage Δ
unittests	88.49% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2c08da...2faaa3b. Read the comment docs.

[Marius1311]

Re the ToDos:

• Writing to AnnData: would actually leave num_exp_genes in .obs - that's really the basis for all of these computations and important for debugging, i.e. to figure out why the score may not work in a particular dataset. As to gene_corr, we can remove that one from .var, I only wrote it there because it was convenient in the further computations to have it there. To summarise, I would write cytotrace, cytotrace_pseudotime and num_exp_genes to .obs and used_for_cytotrace (the old correlates) to .var. I would collect all of these and write in one place, to add some clarity to the function. I already modified the docstring accordingly.
• Parameters to expose: let's not confuse the user here by exposing too much, in particular, I would not expose the number of genes because the authors showed in the original study that 200 genes lead to the best performance on their vast validation set - let's not repeat that validation and let's stick to 200, without exposing. I'm fine with adding a key_added parameter though, however, that should only relate to one of the attributes we write (probably the cytotrace score itself). Also, I think writing parameters to .uns is a good idea.
• how to expose the cytotrace function: I would strongly vote for not exposing this in our public API because it goes against our principle of being all about kernels and estimators. I would really focus everything around this design principle and stick to it quite rigorously. So I think it's okay to make this a class method.

[michalk8]

Re writing to anndata:

Ok, but I'd like to have them all prefixed by e.g. cytotrace_{,pseudotime,num_exp_genes,correlates} (or just cyto_{...} to be short) so that it's more distinguishable (and users know these belong together).
I would also write to .uns the parameters layer, raw and aggregation.

Re params to expose/how to expose:
key_added is not necessary if not exposing it through main API (only through kernel's method), with no n_top_genes I'm also fine.

[Marius1311]

Nice! I like the suggestion of pre-fixing all written attributes. I wouldn't just call it cyto because that just means cell, so let's either go for the full cytotrace or just ct. In the latter case, let's call the score ct_score.

Re the rest, agreed.

Re the anticipated workflow, should a user first call the cytotrace kernel method and then the compute_transition _matrix method?

[michalk8]

Re the anticipated workflow, should a user first call the cytotrace kernel method and then the compute_transition _matrix method?

Currently, the cytotrace-derived pseudotime has to be computed when instantiating the class, because it's subclassing the PT kernel (which initializes ._pseudotime on init). I would stick to this approach, unless you object.
Alt. is to do as you suggest, or call the cytotrace method when calling compute_transition_matrix, but PT kernel has a lot of params there already.

[Marius1311]

I moved the anticipated workflow into the class now - note that for datasets where we don't have spliced/unspliced counts, we currently need to use a hack to get scVelo's moments funciton to work because it needs them. See the workflow in the docstring.

[Marius1311]

Re the anticipated workflow, should a user first call the cytotrace kernel method and then the compute_transition _matrix method?

Currently, the cytotrace-derived pseudotime has to be computed when instantiating the class, because it's subclassing the PT kernel (which initializes ._pseudotime on init). I would stick to this approach, unless you object.
Alt. is to do as you suggest, or call the cytotrace method when calling compute_transition_matrix, but PT kernel has a lot of params there already.

Ok, so that would mean passing all cytotrace parameters when initializing a class object?

[michalk8]

Ok, so that would mean passing all cytotrace parameters when initializing a class object?

Yes, this is currently being done through kwargs.

[Marius1311]

Ok, so that would mean passing all cytotrace parameters when initializing a class object?

Yes, this is currently being done through kwargs.

Can we make sure that these appear in the docstring though? Otherwise, it would be really hard for the user to figure this out.

[michalk8]

Can we make sure that these appear in the docstring though? Otherwise, it would be really hard for the user to figure this out.

Yes, the idea is to write the docstring only in CytoTRACEKernel.compute_cytotrace and use docrep. If you want tab-completion,
we can expose the params (it's only 3) in init as well.

[Marius1311]

Okay, that sounds great, let's expose them in initi as well since it's only 3.

[Marius1311]

Let me know once this is mostly done and I can add some tests.

[michalk8]

Let me know once this is mostly done and I can add some tests.

@Marius1311 done, you can start adding tests.

[Marius1311]

Nice!! Will do!

[Marius1311]

@hspitzer, we're getting there!

[michalk8]

@Marius1311 you can now add the regression test(s).

[Marius1311]

great, thanks!