Pearson Residuals: normalization and hvg #2980
Conversation
| n_cells: int, | ||
| ) -> np.ndarray[np.float64]: | ||
| # TODO: potentially a lot to rewrite here | ||
| # TODO: is there a better/more common way we use? e.g. with dispatching? |
There was a problem hiding this comment.
something I've asked myself multiple times
| return x | ||
|
|
||
| # np clip doesn't work with sparse-in-dask: although pre_res is not sparse since computed as outer product | ||
| # TODO: we have such a clip function in multiple places..? |
| @@ -184,7 +231,11 @@ def _highly_variable_pearson_residuals( | |||
| if clip < 0: | |||
| raise ValueError("Pearson residuals require `clip>=0` or `clip=None`.") | |||
|
|
|||
| if sp_sparse.issparse(X_batch): | |||
| if isinstance(X_batch, DaskArray): | |||
There was a problem hiding this comment.
having _calculate_res_dense and _calculate_res_sparse was there before, maybe singledispatch cleaner here.
the non-jitted dask computation is ~10x slower at the moment.
| sums_cells = axis_sum(X, axis=1, dtype=np.float64).reshape(-1, 1) | ||
| sum_total = sums_genes.sum() | ||
|
|
||
| # TODO: Consider deduplicating computations below which are similarly required in _highly_variable_genes? |
There was a problem hiding this comment.
there's actually quite some duplication, think this might be reduced
| @pytest.mark.parametrize("array_type", ARRAY_TYPES) | ||
| @pytest.mark.parametrize("dtype", ["float32", "int64"]) | ||
| def test_pearson_residuals_inputchecks(array_type, dtype): | ||
| # TODO: do we have a preferred way of making such a small dataset, wich the array types option? |
There was a problem hiding this comment.
copied the array_type(adata.X) from other tests.
is this how we want to set up datasets to test through the combinations?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
==========================================
+ Coverage 75.52% 75.54% +0.02%
==========================================
Files 117 117
Lines 12951 12971 +20
==========================================
+ Hits 9781 9799 +18
- Misses 3170 3172 +2
|
| def custom_clip(x, clip_val): | ||
| x[x < -clip_val] = -clip_val | ||
| x[x > clip_val] = clip_val | ||
| return x |
There was a problem hiding this comment.
Make a utility and use with the other implementation. Also maybe a better name? dask_sparse_compat_clip?
| sums_genes = axis_sum(X, axis=0, dtype=np.float64).reshape(1, -1) | ||
| sums_cells = axis_sum(X, axis=1, dtype=np.float64).reshape(-1, 1) | ||
| sum_total = sums_genes.sum() |
There was a problem hiding this comment.
These functions should work across the types, no? They rely on single dispatch of the first argument and just call the other imeplementations
| def clip(x, min, max): | ||
| x[x < min] = min | ||
| x[x > max] = max | ||
| return x |
There was a problem hiding this comment.
Definitely a feather in the cap for making this a utility.
| def general_max(x, array_type): | ||
| if "dask" in array_type.__name__: | ||
| return x.compute().max() | ||
| return x.max() | ||
|
|
||
|
|
||
| def general_min(x, array_type): | ||
| if "dask" in array_type.__name__: | ||
| return x.compute().min() | ||
| return x.min() |
There was a problem hiding this comment.
better as maybe_compute_{min,max}
There was a problem hiding this comment.
Also can't we check isintance(x, DaskArray)?
| @@ -90,6 +92,47 @@ def clac_clipped_res_sparse(gene: int, cell: int, value: np.float64) -> np.float | |||
| return residuals | |||
|
|
|||
|
|
|||
| def _calculate_res_dense_vectorized( | |||
There was a problem hiding this comment.
Why is this called dense if it operates on sparse inputs? Also why are the type hints np.ndarray then? Also, why the extra dtype? Can we guarantee that?
@ilan-gold, I put some comments where I'm most interested in your feedback for a first step forward