Repository navigation
Conversation
compute_tract_label_indices dropped the tracts shorter than length_threshold from its local list before indexing, so the keys of the four label maps were positions in the filtered list, while the tract bounding boxes, the endpoint positions and the saved tracts use positions in the full tractography. Queries combined unrelated tracts, returned tracts below the threshold, and relative terms raised KeyError. Key the label maps by the position in the full tractography and leave the short tracts out of all of them. Set the bounding boxes and endpoint positions of the left-out tracts to NaN so that relative terms do not select them either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The inversion of a tract -> labels map into a label -> tracts map was written four times: query_processor.labels_for_tracts, compute_label_crossings, compute_label_endings and compute_label_endings_start_end. Move labels_for_tracts to tract_label_indices, where the maps are built, use it in the two functions that build the indices (query_processor imports it from there), and remove compute_label_endings, which nothing calls. No change in behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
What was wrong
The inversion of a tract → labels map into a label → tracts map was written four times (#117):
query_processor.labels_for_tracts,tract_label_indices.compute_label_crossings,compute_label_endingsandcompute_label_endings_start_end.The change
labels_for_tractsmoves totract_label_indices.py, where the maps are built, and is the only implementation.compute_label_crossingsandcompute_label_endings_start_enduse it (the ending maps hold one label per tract, so they are passed as one-element tuples).query_processorimports it from there, soquery_processor.labels_for_tractskeeps working (it is used bytest_query_eval.py); the diff inquery_processor.pyis that import plus the removal of the old definition.compute_label_endingsis removed:git grep -w compute_label_endingsover the whole repository (package,scripts/, tests,doc/) finds only its definition. It is also listed as dead code in ENH: remove dead and broken code #107.No behaviour change.
Testing
TractographySpatialIndexing(crossing_tracts_labels,crossing_labels_tracts,ending_tracts_labels,ending_labels_tracts) are equal before and after this commit, with length threshold 0 and with the 30th length percentile.wmql_1_cst,wmql_2_uf,wmql_3_commissural), length threshold at percentiles 0/30/60: identical tract sets before and after.python -m pytest -q -p no:cacheprovider tract_querier: 43 passed, 1 skipped.make -C doc html) succeeds; no warning involvestract_label_indices.Merge order
Branched from #122 (#46), not from master, because #122 rewrites the same two functions and branching from master would give a hard conflict; until #122 is merged the diff here also shows its commit. Merge after #122; it merges cleanly with #129 and #135 in either order, but is meant to go after them. It also touches
query_processor.py(import + removal at the end of the file), which other open work on that file may need to rebase over.Closes #117
Part of #107
🤖 Generated with Claude Code