fix+perf: audit pass over the post-model phases (10 bugs, 2-6x faster, -50% instance RSS) - #66
Merged
Conversation
A full read of the package looking for correctness bugs, memory bumpers, CPU hotspots and dead code. Ten real defects, a 2-6x speed-up of the two post-model phases, and ~2.4k lines of dead code removed. Bugs (output changes -- each was wrong before): * add_ivd_ep_vert_label used extract_label() without keep_label=True, so the superior/inferior endplate split it had just computed was binarised away and the semantic mask came back with endplates labelled 1 instead of 52/53. * clean_cc_artifacts aliased its dilation with its component mask (np_dilate_msk mutates and returns its input), zeroing the neighbourhood shell *and* the component. The whole only_delete=False path was a no-op that still logged "cleaned". * get_separating_components dilated spart/tpart in place and returned them as "the two separated components" -- they overlapped, and get_plane_split derived its normal vector from their smeared centers of mass. Its erosion loop also collapsed vol/vol_old/vol_erode onto one array. * semantic_bounding_box_clean grew a region to take in nearby connected components and then cropped to the largest component's box anyway. * detect_and_solve_merged_vertebra did `subreg_cc += 100`, lifting the background out of 0 and adding a volume-sized phantom IVD to the stats the split-C2 heuristic reads. * process_dataset logged "stop program" on model incompatibility and continued. * find_prediction_couple detected two partners that agree with the anchor but not with each other, logged that it was skipping them, and used both. * pipeline_version() shelled out to git with no cwd, recording whatever repo the caller stood in into every centroid JSON. Now importlib.metadata; the git revision field is dropped. * filepaths.py ran mkdir into the installed package at import time. * --model-semantic auto / process_dataset(model_semantic=None) always raised NotImplementedError -- the auto-selection was never implemented. Plus: labeling no longer crashes on an empty instance mask or with disable_c1=False and no subregion mask; find_most_probably_sequence no longer mutates the caller's region list and no longer TypeErrors on allow_skip_at_region without region_rel_cost; the labeling classifier patch is read in the model orientation when angle is 0; the seg-key compatibility check compared a list to a list of strings and was always true; the citation reminder now honours its opt-out (renamed SPINEPS_NO_CITATION_REMINDER). Performance (no output change -- gated by voxel-identical golden tests): * The instance phase stores each cutout prediction where it lives instead of in a dense (n_vertebrae, 3, *volume) array, and compares candidates on their overlapping bounding box only. * The endplate splitter runs per vertebra on numpy arrays, grows each dilation by one voxel per round instead of re-dilating from scratch, and uses scipy's binary dilation rather than TPTBox's per-voxel Python loop. * clean_cc_artifacts builds connected components one label at a time, inside each component's bounding box. * The input volume is read from disk once per image instead of up to three times; debug copies are no longer built when they will be discarded. 24 vertebrae, 8.1M voxels: instance 2.22s -> 0.97s (peak RSS +312MB -> +157MB), post-processing 3.20s -> 0.56s. Real TPTBox fixture at 0.75mm: post-processing 6.62s -> 1.44s. Removed: vendored utils/image.py and utils/generate_disc_labels.py, architectures_new/unet2D.py, spineps/example scripts (figures kept), the auto model path and find_best_matching_model, out_unc, parallel_dice, str_id_com_label. The legacy spineps/architectures stack stays -- released checkpoints still load through it. Also fixes a pre-existing pre-commit failure: PLNet.__init__ carried a suppression comment in a form ruff does not recognise, so the pinned ruff 0.11 hook flagged ARG002 on it before this branch too. New tests: unit_tests/test_regression_golden.py pins the instance and post phases on two fixtures by digest; unit_tests/test_bugfixes.py has one test per defect above, each verified to fail on the unfixed code. 219 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013djxckiGFkM46TRjf2RBQW
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.
A full read of the package looking for correctness bugs, memory bumpers, CPU hotspots and dead code. Ten real defects, a 2–6× speed-up of the two post-model phases, and ~2.4k lines of dead code removed.
Everything here is gated by new tests:
unit_tests/test_regression_golden.pypins the instance and post-processing phases on two fixtures by digest, andunit_tests/test_bugfixes.pyhas one test per defect below — each verified to fail on the unfixed code. 219 tests pass, ruff clean, mkdocs builds.Benchmarks
Two fixtures, measured on this branch vs
main(44fe667) with everything else identical.Synthetic whole spine — 24 vertebrae, 8.1M voxels, 0.75 × 0.75 × 1.65 mm (PIR):
predict_instance_mask)phase_postprocess_combined)Real TPTBox test fixture upsampled to 0.75 mm — 1.4M voxels, 3 vertebrae:
The post-processing gain grows with vertebra count and resolution:
add_ivd_ep_vert_labelwas 86% of the phase, and 8.4 s of a 10.6 s profile sat in growing-radius whole-volume dilations.Where the time and memory went
(n_vertebrae, 3, *volume)uint8array that was almost entirely zeros — every prediction only ever covers onecutout_sizewindow. It now stores each cutout where it lives (SparsePrediction), and Dice runs on the intersection of two bounding boxes instead of two whole volumes (~1100 whole-volume passes per scan before).NIIoperators per vertebra per round — and everyNIIoperator copies the array twice (NII_Math._binary_opt). It now works per vertebra on numpy arrays inside that vertebra's own window, grows each dilation by one voxel per round, and uses scipy's binary dilation instead of TPTBox's per-voxel Python loop.clean_cc_artifactsaskednp_connected_components_per_labelfor every label present and held one full-volume component array per label (~25 during the instance merge). It now does one label at a time and works inside each component's bounding box.BIDS_FILE.open_nii()does not cache; the compatibility checks each opened it). Now loaded once and passed in.NoOpDebugSinkdropped the value, but Python still evaluatednii.copy().None of these change output. That is the point of the golden tests: the instance mask and both post-processing outputs are byte-identical across the refactor.
Bugs fixed
These change output, because the previous behaviour was wrong.
add_ivd_ep_vert_labelusedextract_label()withoutkeep_label=True1instead of 52/53.clean_cc_artifactsaliased its dilation with its component masknp_dilate_mskmutates and returns its input, sodilated_m[mask_cc_l != 0] = 0re-read the already-dilated mask and zeroed the shell and the component. The entireonly_delete=Falsepath was a no-op that still logged "cleaned".get_separating_componentsdilatedspart/tpartin placeget_plane_splitthen took its normal vector between their smeared centers of mass. Its erosion loop also collapsedvol/vol_old/vol_erodeonto one array, so the fallback branch could only ever raise.semantic_bounding_box_cleanignored the region it grewdetect_and_solve_merged_vertebradidsubreg_cc += 100--model-semantic autoNotImplementedError— the auto-selection was never implemented. Removed along withfind_best_matching_model.process_datasetmodel incompatibility"stop program"and then carried on. Now raisesValueErrorunless--ignore-model-compatibility.find_prediction_couplepipeline_version()gitwith nocwd, so it recorded whatever repository the caller happened to be standing in into every centroid JSON (or"Version not found"). Nowimportlib.metadata;ctd.info["revision"]is gone.filepaths.pymkdirinto the installed package at import time — fails on a read-only install. Now created on demand.Plus smaller ones: labeling crashed on an empty instance mask and on
disable_c1=Falsewith no subregion mask;find_most_probably_sequencemutated the caller'sregionslist andTypeErrored onallow_skip_at_regionwithoutregion_rel_cost; the labeling classifier read its patch in the wrong axis order whenangle == 0; the seg-key compatibility check compared a list against a list of strings and was always true; the citation reminder ignored its own opt-out (renamed toSPINEPS_NO_CITATION_REMINDER, and it no longer fires on bareimport spineps).Three of these come from one root cause worth knowing about:
np_dilate_mskandnp_erode_mskmodify the array you pass in and return that same object.Removed
Vendored
utils/image.py(697 lines of spinalcordtoolbox) and its only consumerutils/generate_disc_labels.py, a standalone CLI wired to no entry point;architectures_new/unet2D.py(PLNet(do2D=True)now raises — the instance model is 3D only); thespineps/examplescripts (figures kept, the README links them); theautomodel path;out_unc,parallel_dice,str_id_com_label.The legacy
spineps/architecturesstack stays — released checkpoints still load through thetry PLNet / except RuntimeError: PLNet_newpath, which now says so in a comment.Also fixed:
get_models.check_available_modelsassigned its module globals to the same dict objects as the download registry and then mutated them, so a local model scan permanently replaced release URLs with local paths.Not done
get_separating_components'subreg_cc_n == 0fallback now fails with a readable message instead of a bareKeyError, but the algorithm still cannot split a shape with no waist. Reconstructing the intended semantics is guesswork — it needs a domain call.import spinepsis still slow. Droppingentry_pointfrom__init__would not help;api → get_models → seg_modelpulls the whole torch stack regardless.Note for reviewers
spineps/architectures_new/pl_unet.pycarries a one-line fix unrelated to the rest:PLNet.__init__had a suppression comment in a form ruff does not recognise, so the pinned ruff 0.11 pre-commit hook flaggedARG002on it — onmaintoo, not just here.MIGRATION.mddocuments every output change; the README's subregion label table now lists 52/53.🤖 Generated with Claude Code
https://claude.ai/code/session_013djxckiGFkM46TRjf2RBQW