Initial framework of the niche refactor

[shashkat]

Description

Niche refactor improving the current implementation giving the user facing functions a more explicit API, more flexibility through embedder and clusterer classes and possibility of using combinations of them using calculate_niche_custom. Postprocessor classes allow more organized and flexible postprocessing actions like masking and min_niche_size filtering of niches.
The core idea is similar to the spatial_neighbors refactor in #1147 and builds on top of the relatively smaller fix in #1229.

The below graph demonstrates the code flow planned (read children of a node from left to right). This allows usage of different combinations of embeddings and clusterings to obtain niches, and also exposing easy to use and explicit user facing functions for the different flavors. Brown nodes are user-facing functions.

How has this been tested?

Keeping the API of function calculate_niche same, for i) backward compatibility and ii) being able to validate results being same before and after refactor for some test cases that I wrote for the purpose of this refactor, for the different flavors. Though later on, I think calculate_niche should be deprecated as its not clear how arguments of it interact with different values of flavor argument.

Closes

Closes #1192, where this was initially discussed

[codecov]

Codecov Report

❌ Patch coverage is 78.57143% with 63 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.49%. Comparing base (9b81fe7) to head (4ada55e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/squidpy/gr/_niche.py	78.20%	43 Missing and 20 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1245      +/-   ##
==========================================
+ Coverage   77.08%   78.49%   +1.40%     
==========================================
  Files          63       63              
  Lines        9436     9505      +69     
  Branches     1585     1586       +1     
==========================================
+ Hits         7274     7461     +187     
+ Misses       1561     1453     -108     
+ Partials      601      591      -10     

Files with missing lines	Coverage Δ
src/squidpy/_docs.py	95.29% <100.00%> (+0.48%)	⬆️
src/squidpy/gr/_niche.py	76.14% <78.20%> (+38.60%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[shashkat]

This is ready for review @selmanozleyen @grst

Some important points:

• I think one of the most important next steps after this PR is that the naming of the columns added in the different flavors is changed. Currently, column names added look like: adata.obs['nhood_niche_res=1.0']. However, sdata doesn't support such column names with equal-to sign More robust name validation spatialdata#703. Hence, if one used any of the current niche functions with sdata and inplace=True, it will give error. Currently, I kept naming strategy same for backward compatibility, but we should change it after this. It should be relatively straightforward after the refactor.

• I believe the test cases I have added in test_niche.py are quite useful. They helped me validate that the output was the same before and after refactor for an example object (dummy_adata2) for different flavors. However, I realized that with scanpy<1.12.1 in the test env, the output was different than with scanpy>=1.12.1. This is because of a detail in scanpy mentioned here: fix: copy connectivities before passing to umap-learn scanpy#4031, and fixed in 1.12.1 onwards. This means that if someone will run hatch-test on squidpy and they have scanpy<1.12.1 in their test env, some tests will fail. I dont know what is the best way to handle this, hence would like to know your suggestions.

[selmanozleyen]

I will clean up the docstrings to have a dropdown per flavour so we get rid of this mess and will add the AutoK function in a separate PR. But still wanted to ask for a review from you @Intron7

[selmanozleyen]

@shashkat , thanks a lot for the PR I hope you don't mind my commits directly without asking but wdyt?

also you mentioned spatial_neighbours didn't do something as it should like reusing the spatial_neighbours_from_builder or something? I'd like to refactor that

[shashkat]

Hi @selmanozleyen , your welcome! Yeah no problem at all. I went through your changes and they look good to me. Just one point:
This is probably my bad, but in the functions calculate_niche_neighborhood, calculate_niche_utag, calculate_niche_spatialleiden, the order of args in docstrings (resolutions argument) doesn't seem to match the function signature.

Thanks a lot for reviewing the code!

[shashkat]

About the spatial_neighbors_from_builder, this is what we discussed:

[selmanozleyen]

thanks, does this PR include #1229?

[shashkat]

thanks, does this PR include #1229?

Yupp, it does. The first commit in this PR has the same changes in #1229

[selmanozleyen]

Since we are going to deprecate the main functions, I think it's fine to not touch the spatial_neighbours now

[Intron7]

In general I'm a big fan of the new split approach to niche detection. I'm more than happy to adapt the four-function design in rapids-singlecell and deprecate the current calculate_niche — that function has grown unwieldy, and most of _validate_niche_args / _check_unnecessary_args should be deletable once the flavors have their own signatures.

My one ask: the class-based abstraction for custom niche workflows shouldn't be made public in this PR. Keep the classes as the implementation, just private — prefix the seven class names, drop them and calculate_niche_custom from the _niche import block and all in gr/init.py, and drop the matching docs API entries. No logic changes, no re-testing. If we want a discussion about a public class-based custom niche API, that's a fine discussion to have separately, but I'd keep this PR focused on splitting the single function into several.

Separately, and disclosing that both are mine: #1151 and scverse/scanpy#4108 add backend dispatch via scverse-backends, and #1151 dispatches calculate_niche and touches _niche.py, so it will conflict with this. The resolution is that this PR lands first and I rebase — four stable signatures are much better adapter targets than one flavor-branching entry point. But I don't like the class-based choose-your-own-adventure style with dispatch in play. Dispatch attaches to functions the library owns, so a subclass in a third-party package can't be a dispatch target, and get_embedding(adata) -> NDArrayA pins numpy into the contract — which forces materialization back to host at exactly the embed→cluster boundary where a fused implementation would keep things in place. If the ABCs do go public later, that return type should be widened to an array-API protocol first.

[Intron7]

I don't think we should make this costum function public.

[selmanozleyen]

Yeah on the exposure of these custom functions I was doubting myself. I think it's not worth giving these public guarantees for now.

I think the compatibility is a bit better than it was with rsc but compatibility on public layer imo is always going to cause duplicate code and be error prone on long term. I am not saying it should be through the extensibility api's but there should be a layer we give in only array api components as you said. This is the proper solution to it.