Introduce a groupby attribute for correlation computation
#731
Conversation
| @@ -396,7 +396,9 @@ def correlate( # type: ignore[misc] | |||
| self: SpatialMappingMixinProtocol[K, B], | |||
| var_names: Optional[Sequence[str]] = None, | |||
| corr_method: Literal["pearson", "spearman"] = "pearson", | |||
| ) -> Mapping[Tuple[K, K], pd.Series]: | |||
| device: Optional[Device_t] = None, | |||
| groupby: Optional[str] = None, | |||
There was a problem hiding this comment.
what about calling it groups? My understanding is that this is used to compute correlation between groups, see lines 457, 458. I think groups is more fitting and to some extent mirrors the usage of the argument in other scverse packages.
There was a problem hiding this comment.
I actually prefer groupby - you still get a vector of the same length, i.e. length n_genes. The difference to before it that when you compute correlation, you only take into account the cells in the current group. In other words, you now get one such vector (containing correlations) per problem and per group.
I tried to follow scanpy here a bit, see e.g. the violin plotting function, which uses grouopby in a similar fashion. https://scanpy.readthedocs.io/en/stable/generated/scanpy.pl.violin.html
On the other hand, in scanpy's scatter plot function, they use a groups attribute to restrict visualization to certain groups, which is not really what we are doing here: https://scanpy.readthedocs.io/en/stable/generated/scanpy.pl.scatter.html
|
Thanks for the review @giovp, incorporated all your suggestions, except for the |
|
Great, thanks @selmanozleyen @giovp, merging this. |
|
@giovp I think the problem with linting is here for sure. I thought maybe it's a link checking error because of connection but it fails only after this branch (I rerun the checks on older PR's) |
|
yeah I think the problem is something in the docs of the |
|
It's from the warning. Since the |
This introduces a
groupbyparameter formapping_problem.correlate()to evaluate Pearson/Spearmann correlation over groups of cells. I've also adapted tests slightly to cover this case.