Bugfinder (claude) - #121
Closed
Hendrik-code wants to merge 822 commits into
Closed
Conversation
Add new functions and test
… into snapshot_readability
Snapshot readability
fixed binary operator
fix issues with the nnunet slitting script
…ride. (Remove potential sideeffect)
Development robert
fixed licensing mismatch #118
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… into documentation_update
overhauled README, added examples and hopefully made things clearer
Four core operations failed or returned wrong results on ordinary inputs: - NII.rescale(scalar) built a generator, so the following len() raised TypeError. Every scalar-spacing call was broken. - NII.threshold() set arr[arr2>=t]=1 then arr[arr2<=t]=0, so the second write erased the == case; thresholding a binary mask at 1 returned an all-zero image. Rewritten as a single comparison, which also drops a redundant full-volume copy. - NII.normalize() divided by max_out instead of scaling into the target range and offset by min_out/max_out, so normalize(0,255) produced values in [0,1] and tripped its own assert (and max_out=0 raised ZeroDivisionError). Now scales properly, handles constant images, and uses np.isclose instead of exact float equality. - np_dilate_msk(mask=..., use_crop=True) cropped `mask` with the global crop but indexed it against the per-label crop, raising IndexError for any multi-label segmentation - i.e. NII.dilate_msk(mask=...) on its default path. - np_dilate_msk_euclid(labels=...) applied the label filter only when use_crop=False; the default path dilated every label. Both dilate helpers also stopped binarising the caller's mask array in place. Additionally corrects three early-return branches that had `inplace` inverted (nii_wrapper.py:811, 1127, 1140, 2700), so an in-place call no longer returns a different object and an out-of-place call no longer returns an alias of self. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…apped
Three float->int guards used isinstance() against numpy scalar types:
isinstance(self.dtype, np.floating) # a np.dtype object, never a scalar
isinstance(arr, np.floating) # an ndarray, never a scalar
Both are always False, so NII.__init__, NII.set_array and NII.save never
downcast float segmentations. A float64 segmentation therefore stayed
float64 for its whole lifetime - 8x the memory of the uint8 it should be,
through every get_seg_array() copy and onto disk - and it defeated the
unsigned-int fast paths in np_utils. Replaced with np.issubdtype(...).
set_dtype("smallest_int"/"smallest_uint") chose the target type from
arr.max() alone. Since the cast uses casting="unsafe", a negative minimum
wrapped silently (-1 -> 255), and "smallest_uint" fell back to the *signed*
np.int32. Dtype selection is now factored into _smallest_int_dtype(), which
considers both bounds, asserts non-negativity for unsigned targets, and
extends to 32/64-bit.
np_map_labels built its lookup table with dtype=arr.dtype, so any mapping
target outside the input dtype's range wrapped without warning - on a uint8
mask, 1 -> 300 produced 44 and 1 -> -5 produced 251. The table dtype is now
derived from the mapping targets as well; in-range mappings keep the input
dtype as before.
NII.save no longer mutates self via set_dtype_ to achieve its cast, and no
longer re-copies the whole volume afterwards.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…der check np_bbox_binary clamped the stop index to the array shape and only then added 1, so a bounding box touching the far border produced stop == shape + 1. Slicing tolerates that, but every consumer computing stop - start (np_center_of_bbox_binary, NII.compute_crop, is_segmentation_in_border) saw a size one voxel too large. It also stored px_dist in a uint8 array, so any px_dist > 255 raised OverflowError under NumPy 2. Interior boxes are unaffected. np_unique / np_unique_withoutzero are annotated -> list[int] but returned three different scalar types depending on the input dtype and which of the four code paths was taken: Python int from bincount, np.int64 from the withoutzero fast path, and np.float32/np.int16 from the np.unique fallbacks. The numpy scalars are not JSON-serializable, so serializing label lists failed for non-uint inputs. All paths now go through .tolist(), which yields native scalars without truncating float values. is_segmentation_in_border() guarded on `slices is None`, but compute_crop(raise_error=False) returns full-extent slices for an empty mask and never None, so an empty segmentation was reported as touching the border. Now short-circuits on the existing NII.is_empty property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sag_cor_curve_projection did:
order = v_idx_order
order += [i for i in range(256) if i not in v_idx_order]
`order` aliases the module-level v_idx_order list (vert_constants.py), which
is re-exported as TPTBox.v_idx_order, and `+=` extends a list in place. So
rendering a single snapshot permanently grew the shared global from 105 to
256 entries, affecting anything else that reads it. `order` was never used
afterwards, so both lines are dead - removed, along with the now-unused
import.
POI._vert_orientation_pir was a bare class attribute, i.e. one dict shared by
every POI instance in the process. get_vert_direction_PIR compounded this by
writing the cache onto a temporary extract_subregion() copy, so a lookup
could return vertebra directions computed for a different subject processed
earlier. It is now a per-instance dataclass field (default_factory=dict,
excluded from repr/compare, so a copy still starts empty as documented), and
the cache is written to the object the function was called with.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- `from TPTBox import *` raised AttributeError: __all__ advertised load_poi
but nothing imported it. Now re-exported from core.poi_fun.save_load.
- nii[0:5] delegated to a key containing Ellipsis, which __getitem__ itself
rejects two branches earlier, and dropped the `return`. It now pads the
trailing dimensions with full slices.
- extract_label("12") / remove_labels("12"): str is a Sequence, so the str
branch was unreachable and the label was iterated character by character
(IndexError). The str check now precedes the Sequence check.
- ssim()/psnr() normalised via in-place `img_1 /= img_1.max()`, which raises
UFuncTypeError on integer images. The neighbouring img_2 already used the
out-of-place form; both now match.
- sitk_utils.transform_centroid called the non-existent NII.get_empty_POI;
the method is make_empty_POI (used correctly at eight other call sites).
Also drops a duplicated assignment in the deformable branch.
- stitching_tools.n4_bias passed dilate_msk_(mm=3), which is not a parameter
of that method -> TypeError on every call. Now n_pixel=3.
- inference_nnunet forwarded stacklevel= into the logger, which passes
**qargs to print() -> TypeError whenever the input affine is the identity.
- poi_global.to_other tested `isinstance(ref, Self)`; Self is a typing
special form and isinstance() against it raises. Now checks POI_Global,
and the method no longer falls off the end returning None.
- poi_global.__init__ gated level_two_info on level_one_info (copy-paste),
so passing only level_two_info silently dropped it.
- calc_centroids took type(stage) after unwrapping the enum to .value, so
level_one_info/level_two_info were always int and the saved POI header
recorded "int" instead of the enum class.
- BIDS auto_add_run_id did info["run"] += 1 on a value that must remain a
decimal string for validate_entities().
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
None of these raised; they just computed the wrong thing.
vert_constants:
- Full_Body_Instance_Vibe.get_Full_Body_Instance_mapping() referenced
cls.hip_left/hip_right, which do not exist on that enum (they are
pelvis_left/pelvis_right, 50/51, matching the inverse map) -> the whole
classmethod raised AttributeError.
- The same dict literal had duplicate keys: lung_left twice, lung_right three
times, channel twice. Only the last of each survived, so the shadowed
entries were dead. Removed them, keeping the previously-winning target so
behaviour is unchanged, with a note that one FBI lung label cannot address
several Vibe lobes.
- Abstract_lvl._get_id passed `cls` positionally into a classmethod, shifting
the arguments and raising TypeError on every lookup - swallowed by a bare
`except Exception`. Name resolution therefore never worked:
Any._get_id("L1") fell through to int("L1"). Now resolves to 20.
np_utils:
- np_filter_connected_components compared a list[tuple] to an int, so that
shortcut branch was unreachable. NOTE: fixing this to len(...) changes
which components are preserved when the count equals largest_k_components;
worth a domain review.
- The background_threshold in the label-smoothing helper was applied to the
argmax *indices* rather than the winning confidence, so it removed labels
by index rather than by probability.
Other:
- vertebra_direction dropped the result of
set_array().reorient().rescale_(), then read the array back off the
un-reoriented object, writing the fill-back image in the wrong orientation
and spacing. Mirrors the correct chained form in calc_center_spinal_cord.
- SegmentationMesh discarded `int_arr.astype(np.uint16)`, so the float->int
conversion it announces never happened.
- snapshot_modular: cmap(color - 1 % LABEL_MAX % cmap.N) parses as color - 1,
losing the wrap-around; parenthesised to match the correct site above it.
- angles: `if (vert, 50) not in poi: cord = poi[vert, 50]` was inverted, so
no lordosis/kyphosis label was ever emitted.
- body_quadrants requested Vertebra_Direction_Inferior twice but reads
Vertebra_Direction_Right; the KeyError was swallowed, so every vertebra was
skipped and an all-zero image returned.
- inference_nnunet built its label mapping with the raw string key instead of
the parsed int.
- predictor accumulated the chunk bounding-box max from self.min_s.
- deepali_model gated source_seg on fixed_seg and target_seg on moving_seg.
- point_registration.load_ assigned the dumped (moving, fixed) pair to
(_img_fixed, _img_moving), so reloaded registrations mapped the wrong way.
- save_mkr passed split_by_region=split_by_subregion, so with the documented
defaults neither branch was taken and markers got random colours.
- ray_casting negated z instead of flipping the half-space inequality.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…copies
Import cost: nii_wrapper_math imported peak_signal_noise_ratio and
structural_similarity from skimage.metrics at module level, but they are used
only inside NII.ssim and NII.psnr. That import pulls in scipy.stats, which
accounted for ~35% of `import TPTBox`. Moved both into their methods,
following the existing skimage.exposure precedent in nii_wrapper.py.
import TPTBox 948 ms -> 613 ms (scipy.stats no longer loaded at all)
Memory:
- vertebra_direction allocated `subreg_iso.get_array() * 0` once per vertebra:
get_array() copies the whole volume and `* 0` allocates a second one. Now a
single np.zeros of the same shape and dtype. Two sites.
- set_dtype called get_array() (a full copy) twice on the smallest_int/uint
path; it is now fetched at most once.
Resource leaks:
- _help.py created a figure that was never closed - one leaked figure per call.
- snapshot_modular used a bare plt.close(), which closes the *current* figure
rather than the one just saved, and was skipped entirely if savefig raised.
Both now close their own figure in a finally block.
Encoding: the Logger opened its .log with the platform default encoding, so
print_statistic's "±" raised UnicodeEncodeError under a C/POSIX locale
(Docker, cron, CI). Its logs directory is also created with
parents=True, exist_ok=True, which parallel jobs were racing on. The same
missing encoding= is fixed for the POI, BIDS-sidecar and DICOM JSON readers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TPTBox/registration/ridged_intensity/ was meant to be renamed to _ridged_intensity/ in e919218 ("rename folder to _ to show that this is not the recommended way to import things"), but the old directory stayed tracked. Both copies then received the docstring pass in 81db55a, while only the underscore copy received 1b8e0fb ("fix bug for very elongated segmentations") - so the duplicate still carries the pre-fix `w = max(target.shape[2:])` and the un-scaled delta comparison. Nothing imports it and its __init__.py is empty, so it is removed, along with the stale __pycache__-only leftovers at registration/{deepali,deformable,ridged_points}/. The optional-dependency guards in registration/__init__.py and _deepali/__init__.py caught bare `Exception`, which makes a real NameError or AttributeError inside those modules indistinguishable from "torch is not installed" - the symbol just silently disappears from the public API. That is how the stale duplicate above, and the NameError in deepali_trainer, went unnoticed. Narrowed to ImportError; all five exported names still resolve. Also drops the duplicate MODES definition in nii_wrapper.py, which shadowed the identical import 13 lines above it, and two leftover debug prints - one in POI buffer loading, one emitted once per bisection step per point from ray_casting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
3 participants
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.
Summary
Repository-wide audit of TPTBox (~55k LOC / 180 files) for correctness bugs, performance problems, excess memory use>
Every finding was verified by executing it against the installed package or by reading the exact source lines; claim>
The headline result: four core
NIImethods were broken on their default paths, and three `isinstance(..., np.flo>Broken on default paths
NII.rescale(1.5)len()on it →TypeErroron every scalar-spacing callNII.threshold(1)>=t → 1then<=t → 0; the second write erased the==case, so a binary mask thres>NII.normalize(0, 255)max_outinstead of scaling into the range → values in [0,1] and a failing>NII.dilate_msk(mask=…)maskcropped with the global crop but indexed against the per-label crop → `IndexErro>Silent data corruption
isinstance(self.dtype, np.floating)is alwaysFalse(.dtyperet>np_map_labelswrapped on dtype overflow. The lookup table was built withdtype=arr.dtype, so on a uint8 ma>TPTBox.v_idx_orderwas corrupted globally.sag_cor_curve_projectionaliased the module-level list and+=>POI._vert_orientation_pirwas a class attribute, i.e. one dict shared by every POI in the process, written v>np_dilate_msk_euclid(labels=…)applied the label filter only whenuse_crop=False; the default path dilated eve>np_bbox_binaryclamped before adding 1, so a box touching the far border returnedstop == shape + 1; `px_dist >set_dtype("smallest_int"/"smallest_uint")selected frommax()alone, so negatives wrapped silently under `cast>np_uniquereturned three different scalar types depending on dtype and path; the numpy ones are not JSON-seriali>Point_Registration.load_assigned the dumped(moving, fixed)pair to(_img_fixed, _img_moving)— every reloa>background_thresholdapplied to argmax indices rather than confidence; alist == intcomparison making>Crashes
from TPTBox import *raisedAttributeError(__all__advertisedload_poi, nothing imported it) ·nii[0:5]·>Performance & memory
import TPTBox: 948 ms → 613 ms (−35%).nii_wrapper_mathimportedskimage.metricsat module level for two>vertebra_directionallocated two full volumes per vertebra viaget_array() * 0→ onenp.zeros.set_dtypecopied the whole volume twice;savecopied it up to four times and mutatedself.snapshot_modular's bareplt.close()closed the current figure and was skipped i>Hygiene
TPTBox/registration/ridged_intensity/— the un-renamed duplicate from2f5078f. Nothing imports it, its>except Exception: passtoexcept ImportError:around optional-dependency imports — catching bare `Exc>Loggeropened its.logwithoutencoding=, soprint_statistic's±raisedUnicodeEncodeErrorunder a C/P>MODESdefinition and two leftover debug prints (one fired once per bisection step, per point>Three things that need your judgement
np_filter_connected_components—label_volume_pairs == largest_k_componentscompared alist[tuple]to a>get_Full_Body_Instance_mapping— oneFull_Body_Instancelung label cannot address >weakref.finalizehandle retention inlog_file.py(fixing it means restructuri>Test plan
🤖 Generated with Claude Code