spatial_neigbours extensibility features and clarification#1147
spatial_neigbours extensibility features and clarification#1147selmanozleyen wants to merge 47 commits intoscverse:mainfrom
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 74.05% 74.31% +0.25%
==========================================
Files 39 40 +1
Lines 6495 6658 +163
Branches 1122 1123 +1
==========================================
+ Hits 4810 4948 +138
- Misses 1230 1253 +23
- Partials 455 457 +2
🚀 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!
shashkat
left a comment
shashkat
left a comment
There was a problem hiding this comment.
Thanks a lot for doing this :)
Here are some minor suggestions from me!
src/squidpy/gr/neighbors.py
Outdated
| def build_graph(self, coords: NDArrayA) -> tuple[csr_matrix, csr_matrix]: | ||
| N = coords.shape[0] | ||
| r = self.radius if isinstance(self.radius, int | float) else max(self.radius) | ||
| tree = NearestNeighbors(n_neighbors=self.n_neighs, radius=r, metric="euclidean") |
There was a problem hiding this comment.
Since we are using tree.radius_neighbors() below, is the n_neighbors = self.n_neighs having an effect here?
Is it intentional to have some of these arguments still valid for RadiusBuilder, DelaunayBuilder etc? I see that for the parameter n_neighs, there is a warning in DelaunayBuilder that it will be ignored. If its intentional to keep these arguments for now, then possibly can add warning for n_neighs in RadiusBuilder too.
There was a problem hiding this comment.
you are right it was a legacy thing. I removed it now
| "transform": transform, | ||
| "set_diag": set_diag, | ||
| } | ||
|
|
There was a problem hiding this comment.
WRT the removal of non necessary arguments from RadiusBuilder, DelaunayBuilder etc, whenever it is decided to remove them (now or later), it would also be nice to make the logic here more explicit. By more explicit logic I mean if else hierarchy closely resembling the decision tree we have possible. I made a rough version according to my understanding:
There was a problem hiding this comment.
Since all of this is deprecated anyway, I wouldn't put too much effort into making this part "nice"
grst
left a comment
grst
left a comment
There was a problem hiding this comment.
It's taking a nice shape now... mostly minor things at this point!
| "transform": transform, | ||
| "set_diag": set_diag, | ||
| } | ||
|
|
There was a problem hiding this comment.
Since all of this is deprecated anyway, I wouldn't put too much effort into making this part "nice"
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
|
Thanks a lot @grst ! I really appreciate your reviews. I assesed them and even made a more general class to support Cupy arrays in future. |
3 participants
fixes: #1102 and #1047
It's backward compatible and I am curious what the community might bring to this!