Replace spmatrix with _CSMatrix as appropriate #3431
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3431 +/- ##
==========================================
+ Coverage 76.67% 76.70% +0.03%
==========================================
Files 112 112
Lines 12945 12957 +12
==========================================
+ Hits 9925 9939 +14
+ Misses 3020 3018 -2
|
There was a problem hiding this comment.
Looks good, about the plan:
replace spmatrix with _SpBase = sparray | spmatrix and _CSMatrix with _CSBase = cs{cr}_matrix | cs{cr}_array and test
I am not sure this will go so smoothly because of differing APIs and version compatibility. It was something of a nightmare in AnnData...
| def _resolve_vals( | ||
| val: NDArray | sparse.spmatrix | DaskArray, | ||
| ) -> NDArray | sparse.csr_matrix | DaskArray: | ||
| msg = f"Unsupported type {type(val)}" | ||
| raise TypeError(msg) |
There was a problem hiding this comment.
Does this type hint make sense? Why not Any?
| @@ -298,9 +298,9 @@ def calculate_qc_metrics( | |||
| ) | |||
| # Pass X so I only have to do it once | |||
| X = _choose_mtx_rep(adata, use_raw=use_raw, layer=layer) | |||
| if isspmatrix_coo(X): | |||
| if isinstance(X, spmatrix) and not isinstance(X, _CSMatrix): | |||
There was a problem hiding this comment.
This check seems different than the one before, and I'm not sure we want to be more permissive
| @@ -105,28 +105,33 @@ def preprocess_with_pca(adata, n_pcs: int | None = None, random_state=0): | |||
|
|
|||
|
|
|||
| def get_init_pos_from_paga( | |||
| adata, adjacency=None, random_state=0, neighbors_key=None, obsp=None | |||
| adata: AnnData, | |||
| adjacency: spmatrix | None = None, | |||
There was a problem hiding this comment.
Was the spmatrix here intentional? I could see an argument for it because this function doesn't use it (so this type is permissive) but also, it fetches it out of obsp if it's not present suggesting that it has an "expected" type
Preparation for sparray support: make sure
spmatrixis only used where it makes sense.I checked every single instance of
spmatrixand either tested thatcoo_matrixalso works or replaced it with_CSMatrix = csr_matrix | csc_matrix.TODO after this PR:
_SpBase = sparray | spmatrixand_CSMatrixwith_CSBase = cs{cr}_matrix | cs{cr}_arrayand testbsr_array