Feedback on new niche implementation

Hi @shashkat and @selmanozleyen,

thanks for refactoring the niche stuff! I wasn't available to comment on https://github.com/scverse/squidpy/pull/1245 in time, so I'll leave some implementation-specific comments in this issue, to be addressed in a follow-up PR.

General

• please use kwargs-only for basically all arguments except data for all public functions
• in general, the docstrings of the different niche methods are a bit light on details. I think they should provide a bit more details on the methodology, how they compare one to another, and reference the original publications. The docstrings should also make it clear to the user e.g. when to prefer UTAG over Neighborhood.
• You sometimes use logg.warning, and sometimes warnings.warn. Is that intentional?

calculate_niche_neighborhood

• docstring calculate_niche_neighborhood -> specify what happens to the output if multiple resolutions are specified.

Neighborhood embedder

• if extending n_hop_weights, I think this would most likely not intentional. I'd at least emit a warning, not debug, but probably just fail.
• _calculate_neighborhood_profile. I think most of the calculation could be simplified into pd.dummies + matrix multiplication (
  (pd.get_dummies(adata.obs[self.groups]).T.values @ adata.obsp[self.spatial_connectivities_key].T).T)
• I wouldn't prematurely convert the profile to a pandas df as long as you still perform arithmetic operations on it.

UTAG

• it should be possible to specify an AnnData layer, do not blindly use X.
• Utag embedder: sc.tl.pca can take a sparse matrix directly and return the result matrix. No need to create an intermediate AnnData object.

Cellcharter

• I would move use_rep outside the _CellcharterEmbedder, i.e. intantiate the embedder only if use_rep is None. The embedder could be renamed to something more generic, such as NHopPCAEmbedder.
• why the _setdiag with .tolil() -> .tocsr() roundtrip? CSR matrices have a native .setdiag function.
• not exactly sure what cellcharter's intention with the "visited" filter is. If a k-hop space should strictly exclude nodes that are already included in a (k-1)-hop-space , this doesn't work since it is not accounted for node degrees. The comparison adj_hop = adj_hop > adj_visitied would need to use boolean matrices instead.

_postprocess_niche_results

• I'd convert the result columns back to categoricals after assigning the prefixes.

_calculate_niche_custom

Not entirely sure about this, but could it make sense, for performance reasons, to move the embedder out of the loop? The spatial neighbohood graph should already account for libraries. The embedding could then be stored globally in adata.obsm and be inspected or potentially be reused for other purposes.