spatial_neigbours extensibility features and clarification#1147
spatial_neigbours extensibility features and clarification#1147selmanozleyen wants to merge 78 commits into
Conversation
for more information, see https://pre-commit.ci
…leyen/squidpy into feat/spatial_neighbours
for more information, see https://pre-commit.ci
…leyen/squidpy into feat/spatial_neighbours
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1147 +/- ##
==========================================
+ Coverage 73.82% 74.12% +0.30%
==========================================
Files 45 46 +1
Lines 7013 7195 +182
Branches 1188 1193 +5
==========================================
+ Hits 5177 5333 +156
- Misses 1349 1371 +22
- Partials 487 491 +4
🚀 New features to boost your workflow:
|
…leyen/squidpy into feat/spatial_neighbours
for more information, see https://pre-commit.ci
grst
left a comment
grst
left a comment
There was a problem hiding this comment.
Thanks @selmanozleyen, that goes in the right direction. I put up some design questions up for discussion!
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
shashkat
left a comment
shashkat
left a comment
There was a problem hiding this comment.
It looks amazing to me now! Just a few more minor suggestions. I really liked the idea of using specialized Postprocessor classes!
|
These classes would be also ideally serializable so we dont need to rely on a forman under adata.uns which we are scared to change. I will make this as an issue to tackle when the time comes |
timtreis
left a comment
timtreis
left a comment
There was a problem hiding this comment.
We're not really explicitly testing the new pattern, just indirectly through the equvialence tests.
Also, we should align on the deprecate warning version
Claude:
Extensibility doc: SNN example will not import / run as written
docs/extensibility.md SNNRadiusBuilder.build_graph doesn't initialise the matrices' set_diag consistently with the rest of the codebase — adj.setdiag(1.0 if self.set_diag else adj.diagonal()) will materialise an explicit zero diagonal because
csr_matrix.diagonal() of a freshly constructed matrix returns a dense vector of zeros, then setdiag(0.0) is set on dst (fine). Minor, but worth either (a) testing this snippet in CI as a doctest, or (b) simplifying to mirror what
RadiusBuilder.build_graph actually does. Also the example imports snnpy (optional 3p dep) and silently breaks doc-build if anyone runs the snippet — flag clearly that it is illustrative.
| "connectivities_key": conns_key, | ||
| "distances_key": dists_key, | ||
| "params": { | ||
| "n_neighbors": getattr(builder, "n_neighs", 6), |
There was a problem hiding this comment.
Why is 6 hardcoded? Not all method have 6 neighbors
| elements_to_coordinate_systems: dict[str, str] | None = None, | ||
| table_key: str | None = None, | ||
| library_key: str | None = None, | ||
| radius: float | tuple[float, float] = 1.0, |
There was a problem hiding this comment.
Why the default of 1 here? Isn't it None in the others?
There was a problem hiding this comment.
you are right, I made it a non-defaulted positional arg
There was a problem hiding this comment.
FWIW, it's also possible to have keyword-only arguments without default.
| elements_to_coordinate_systems: dict[str, str] | None = None, | ||
| table_key: str | None = None, | ||
| library_key: str | None = None, | ||
| n_neighs: Literal[4, 6] = 6, |
There was a problem hiding this comment.
From the docs what I understood is only those two makes sense
4 participants
fixes: #1102 and #1047
It's backward compatible and I am curious what the community might bring to this!