fix: use P-axis length for ys calculation and clip S-axis bounds (#167) - #180
fix: use P-axis length for ys calculation and clip S-axis bounds (#167)#180rtmalikian wants to merge 4 commits into
Conversation
Sorry, something went wrong.
Fixes part 2 of BrainLesion#167: - Use mask_RPS.shape[1] (P-axis) instead of shape[2] (S-axis) for ys calculation, since the defacing line iterates over P positions and computes S-axis limits for each. - Clip y values to mask_RPS.shape[2] to prevent IndexError when the defacing line extends beyond the S-axis range. Previously, using shape[2] for the arrange caused the defacing loop to iterate over S-axis positions instead of P-axis positions, which produced incorrect defacing masks for images with narrow S axes. The old code also lacked bounds clipping, causing IndexError when computed S-axis limits exceeded the array dimensions.
There was a problem hiding this comment.
Pull request overview
This pull request fixes an indexing bug in the QuickShear defacing implementation by computing the defacing-plane ys values across the correct axis (P-axis) and clipping S-axis slice bounds to prevent out-of-range writes.
Changes:
- Compute ys using mask_RPS.shape[1] (P-axis length) instead of mask_RPS.shape[2] (S-axis length).
- Clip computed S-axis limits (y) to mask_RPS.shape[2] before slicing to avoid out-of-bounds access.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
@uturkbey @nicmuenster do you have time to look into this? |
Sorry, something went wrong.
|
im not familiar with the details; if this should be a valid fix, it should also be raised in the reference implementation: https://github.com/nipy/quickshear/blob/master/quickshear.py |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)brainles_preprocessing/defacing/quickshear/nipy_quickshear.py:163
- The loop pairs x indices where ys > 0 with unfiltered ys.astype(int) values. If ys is not positive from index 0 onward, zip(np.nonzero(ys > 0)[0], ys.astype(int)) will misalign x and y (e.g., first positive x will be paired with ys[0]), producing an incorrect defacing mask. Filter ys by the same indices (or index into ys inside the loop) so each x uses its corresponding y.
for x, y in zip(np.nonzero(ys > 0)[0], ys.astype(int)):
y_clipped = min(y, mask_RPS.shape[2])
defaced_mask_RPS[:, x, :y_clipped] = 0
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Summary
Fixes the second part of #167: incorrect ys axis calculation and missing bounds clipping in QuickShear defacing.
Root Cause
The ys calculation in run_quickshear() used mask_RPS.shape[2] (S-axis length) instead of mask_RPS.shape[1] (P-axis length):
In RPS orientation, the defacing loop iterates over P-axis positions (axis 1) and computes S-axis limits (axis 2) for each. Using shape[2] caused:
Fix
Verification
Tested with 4 scenarios including the exact shapes from the bug report:
All tests pass with no IndexError and correct defacing behavior.
About the Author: Raphael Malikian — Clinical AI Solutions Architect. I specialise in building and fixing AI/ML systems for healthcare, including vector databases, RAG pipelines, and clinical NLP. If you need help with your project or think I can add value to your organisation, feel free to reach out — I'd love to connect.
📧 rtmalikian@gmail.com
🔗 GitHub: https://github.com/rtmalikian
🔗 LinkedIn: http://www.linkedin.com/in/raphael-t-malikian-mbbs-bsc-hons-71075436a
Disclosure: This code was developed with assistance from mimo-2.5-pro (Xiaomi) via Hermes Agent (Nous Research). All changes were reviewed, tested against the actual codebase, and verified for correctness.