theislab / scib

Benchmarking analysis of data integration tools
MIT License
294 stars 63 forks source link

use_rep in cluster_optimal_resolution and opt_louvain #389

Open kzkedzierska opened 11 months ago

kzkedzierska commented 11 months ago

Hiya!

First off, thanks a ton for this awesome package – it’s super helpful!

There seems to be some inconsistency across those two functions: cluster_optimal_resolution and opt_louvain. In opt_louvain, even if use_rep is specified when adata.uns["neighbors"] is already present, the embedding argument is ignored (here). However, in the case of cluster_optimal_resolution the function doesn't look for adata.uns["neighbors"] when use_rep is specified (here).

I noticed that opt_louvain is deprecated, but I think it would be useful to raise a warning when use_rep is used while adata.uns["neighbors"] is already there. The docs do mention that use_rep gets ignored in that scenario, but it still escaped me and caused me a bit of a headache. I thought that maybe a warning could save the next person.

On another note - in the case of cluster_optimal_resolution, if I'm correct, the docstring mentioning "only if adata.uns['neighbors'] is not defined, otherwise will be ignored" might be a bit misleading.

If you are ok with it and it would be of any help I can submit a PR.