fix(skeletonize): correct edge orientation, dataset routing, and soma detection - #24
Merged
Merged
Conversation
… detection Correctness: - _create_node_info_dict: derive parent pointers via BFS over undirected adjacency instead of trusting edge direction. meshparty emits [child, parent] and CAVE edges may be unordered, so `parent, child = edge` inverted roots and overwrote children of branch points, producing fragmented neurons. Now guarantees one parent per node and one root per connected component. - detect_soma_skeleton: `not any(is_big)` tested index values; use `is_big.size == 0` so a large node at position 0 is no longer skipped. - skeletonize_neuron / skeletonize_neurons_parallel: add a real `dataset` parameter so @inject_dataset works, and fetch the mesh from the matching segmentation source (get_cloudvolume(dataset=dataset)) instead of always the default dataset; thread dataset through get_skeletons and the recursive branch. - skeletonize_neuron iterable branch: treat save_to as a directory and write one <root_id>.swc per neuron instead of overwriting a single file. - get_skeletons: dedup root_ids (pd.unique) so duplicates aren't fetched twice and the id-based reindex stays unambiguous. Performance: - detect_soma_mesh: replace the O(n^2) per-vertex tree.query(k=n) loop with a single vectorized query_ball_point(..., return_length=True). Robustness / observability: - get_skeletons: log the precomputed-fetch failure at debug instead of a silent `except Exception: pass`; iterate futures with as_completed and report the failing root id. - color_map: use plt.get_cmap (cm.get_cmap is removed in modern matplotlib) and size colors to the surviving neurons. - _worker_wrapper: brief backoff before the single network retry; stop swallowing KeyboardInterrupt. Cleanup: - _shave_skeleton: use navis.subset_neuron instead of mutating tn._nodes. - Remove dead code (unused coords/partial/cm imports, redundant local re-imports, duplicate np.asarray, no-op try/except, dead failed_ids). Tests: - Add tests/test_skeletonize.py: offline unit tests for the dict-to-TreeNeuron conversion, soma detection regressions, detect_soma_mesh neighbor-count equivalence, and the get_skeletons dedup contract. No CAVE credentials required. Docs: - Add changelog entries under [Unreleased].
Contributor
There was a problem hiding this comment.
Pull request overview
Hardening pass on crantpy.viz.skeletonize’s skeleton-fetch/skeletonize path to improve correctness (edge orientation, dataset routing, soma detection), reduce a soma-mesh performance hotspot, and add offline unit tests to lock in behavior without requiring CAVE credentials.
Changes:
- Rebuild
_create_node_info_dictparent pointers from undirected adjacency (BFS) to make skeleton topology orientation-invariant and prevent dropped branch children. - Thread
datasetthroughget_skeletons/skeletonize_neuron/skeletonize_neurons_parallelso on-demand mesh fetching uses the correct dataset. - Fix soma detection regressions and vectorize
detect_soma_meshneighbor counting; add a comprehensive offline test suite for these helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/test_skeletonize.py |
Adds offline unit tests covering SWC conversion/topology invariance, soma detection regressions, and get_skeletons dedup/ordering behavior. |
src/crantpy/viz/skeletonize.py |
Fixes topology construction, threads dataset through mesh fallback paths, improves failure reporting/concurrency behavior, and optimizes soma mesh detection. |
docs/changelog.md |
Documents the fixes, new dataset parameter plumbing, and the new offline test coverage. |
Comments suppressed due to low confidence (1)
src/crantpy/viz/skeletonize.py:496
_worker_wrappercan return a structured errordict, but its return type annotation isUnion[navis.TreeNeuron, str]. This is misleading for callers (and for static analysis) and doesn’t match the actual implementation/docstring.
def _worker_wrapper(
x: Tuple[Callable, List[Any], Dict[str, Any]],
) -> Union[navis.TreeNeuron, str]:
"""worker wrapper (from fafbseg) with error handling and retry logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+487
to
+490
| warnings.warn( | ||
| "assert_id_match is not implemented yet and currently has no effect.", | ||
| category=UserWarning, | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Hardening pass on
crantpy.viz.skeletonize(theget_skeletons/skeletonize_neuronpath). Fixes a skeleton-topology bug, a wrong-dataset bug, and a soma-detection bug, removes an O(n²) hotspot, and adds offline tests. Public API is unchanged.Highlights
_create_node_info_dicttrusted edge direction (parent, child = edge), but meshparty emits[child, parent]and precomputed CAVE edges may be unordered → inverted roots and branch points silently losing children, producing fragmented neurons. It now rebuilds parent pointers via a BFS over undirected adjacency: exactly one parent per node and one root per connected component. Affects both thepcg_skeland precomputed-fetch paths.get_cloudvolume()with no dataset, so meshes always came from the default segmentation regardless of the client's dataset. Added a realdatasetparameter (so the existing@inject_datasetdecorator actually works) and threaded it end-to-end (get_skeletons→skeletonize_neuron→get_cloudvolume(dataset=...)).detect_soma_skeletonskipped a segment whose only large-radius node sat at index 0 (not any(is_big)tested index values, not emptiness).detect_soma_meshreplaced an O(n²) per-vertextree.query(k=n)loop with a single vectorizedquery_ball_point(..., return_length=True).save_tonow writes one<root_id>.swcper neuron instead of overwriting a single file;get_skeletonsdeduplicatesroot_ids, logs the previously silently-swallowed precomputed-fetch error, and iterates withas_completed(per-id failure messages);color_mapusesplt.get_cmap(the oldcm.get_cmapis removed in modern matplotlib) and sizes colours to the surviving neurons;_shave_skeletonusesnavis.subset_neuroninstead of mutatingtn._nodes; dead code removed.Type of change
datasetis an additive optional parameter;__all__is unchanged.Testing
tests/test_skeletonize.py: 21 offline unit tests (no CAVE credentials) covering the dict→TreeNeuronconversion (orientation invariance, branch-child retention, disconnected components, type classification), the soma-detection regressions, thedetect_soma_meshneighbour-count equivalence, and theget_skeletonsdedup/ordering contract. All pass.blackclean;rufferrors on this file reduced from 13 → 6 (all remaining are pre-existing in untouched code; this change introduces none).build_docs.sh(Poetry not installed locally). The docs CI builds on PR withexecute_notebooks: 'off', and onlydocs/changelog.mdchanged (API docs are autodoc).Checklist
__init__files unchanged (mkinitis a no-op;__all__identical)docs/changelog.md,[Unreleased])Related issues
None.