-
Notifications
You must be signed in to change notification settings - Fork 60
Features/1900 image transformations beyond fft #2048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
505a7e0 to
9bdd8d2
Compare
|
@markak47 thank you so much for the work and examples, this looks very nice already. Before we go on with optimizing the implementation, a few tips on integrating this into the Heat codebase:
This is all a bit boring, but if we leave it for later, it'll be much more work. Thank you so much Mark! |
…ary files module name change
…sary files, module name change
…sary files, module name change
1e81333 to
188bc5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds new affine transformation functionality to the Heat ndimage module, enabling 2D and 3D image transformations including rotation, scaling, translation, and shearing. The implementation uses backward warping with support for different interpolation methods (nearest neighbor and bilinear) and multiple padding modes (constant, nearest, wrap, reflect).
Key Changes:
- New
affine_transformfunction supporting 2D (2x3) and 3D (3x4) transformation matrices with backward warping semantics - Nearest neighbor and bilinear interpolation modes with customizable boundary handling
- Comprehensive test suite validating transformations, interpolation, and padding behaviors
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| heat/ndimage/affine.py | Core implementation of affine transformation with coordinate mapping, interpolation methods, and padding modes |
| heat/ndimage/tests/test_affine_nd.py | Unit tests covering 2D/3D transformations, identity, translation, rotation, scaling, interpolation, and padding modes |
| heat/ndimage/examples/mri_testscan.py | Example demonstrating affine transformations on MRI data with rotation, scaling, translation, and shearing |
Comments suppressed due to low confidence (2)
heat/ndimage/examples/mri_testscan.py:79
- Variable cx is not used.
cx, cy = W/2, H/2
heat/ndimage/examples/mri_testscan.py:79
- Variable cy is not used.
cx, cy = W/2, H/2
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
heat/ndimage/affine.py
Outdated
| mode="nearest", | ||
| constant_value=0.0, | ||
| expand=False, # kept for later; currently no-op | ||
| ): |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order parameter is not validated. Invalid values (e.g., order=5) will cause the function to silently use bilinear interpolation via the else branch at line 235. Add validation to ensure order is either 0 (nearest) or 1 (bilinear), and raise a ValueError for unsupported interpolation orders.
| ): | |
| ): | |
| if order not in (0, 1): | |
| raise ValueError(f"Unsupported interpolation order: {order}. Only 0 (nearest) and 1 (bilinear) are supported.") |
examples/mri_testscan.py
Outdated
| """ | ||
| Input: 2x3 affine matrix. | ||
| Output: 2x3 affine matrix recentered around the image center. | ||
| """ |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation of the docstring is inconsistent with the function definition. The opening triple quotes should align with the function's indentation level (8 spaces), not have extra indentation (12 spaces).
| """ | |
| Input: 2x3 affine matrix. | |
| Output: 2x3 affine matrix recentered around the image center. | |
| """ | |
| """ | |
| Input: 2x3 affine matrix. | |
| Output: 2x3 affine matrix recentered around the image center. | |
| """ |
| b = torch.tensor(M[:, ND:], device=device).reshape(ND, 1) | ||
|
|
||
| # Backward warp matrix | ||
| A_inv = torch.inverse(A) |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matrix inversion at line 199 can fail if the transformation matrix A is singular (non-invertible). This will raise an exception from torch.inverse, but without a clear error message explaining the issue. Consider adding a try-except block to catch this case and provide a more informative error message to users about why their transformation matrix is invalid.
examples/mri_testscan.py
Outdated
| # Define transforms (all center-aware) | ||
| # ======================================================== | ||
|
|
||
| cx, cy = W/2, H/2 |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables cx and cy are defined but never used (they are redefined within the recenter function at line 88). Consider removing this line to avoid confusion.
| cx, cy = W/2, H/2 |
| def _normalize_input(x, ND): | ||
| orig_shape = x.shape | ||
| t = x.larray | ||
|
|
||
| # 2D image: (H, W) or (C, H, W) | ||
| if ND == 2: | ||
| if x.ndim == 2: # (H,W) | ||
| t = t.unsqueeze(0).unsqueeze(0) # → (1,1,H,W) | ||
| elif x.ndim == 3: # (C,H,W) | ||
| t = t.unsqueeze(0) # → (1,C,H,W) | ||
|
|
||
| # 3D image: (D,H,W) or (C,D,H,W) | ||
| else: | ||
| if x.ndim == 3: # (D,H,W) | ||
| t = t.unsqueeze(0).unsqueeze(0) # → (1,1,D,H,W) | ||
| elif x.ndim == 4: # (C,D,H,W) | ||
| t = t.unsqueeze(0) # → (1,C,D,H,W) | ||
|
|
||
| return t, orig_shape |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _normalize_input function doesn't handle invalid input dimensions. For 2D transforms (ND=2), if the input has ndim != 2 and ndim != 3, the function returns the original tensor without proper normalization. Similarly for 3D. This will cause errors downstream in the sampling functions. Add an else clause to raise a ValueError with a clear message about expected dimensions.
heat/ndimage/tests/test_affine_nd.py
Outdated
| def test_expand_rotation(self): | ||
| x = ht.array([ | ||
| [1.,0.], | ||
| [0.,1.] | ||
| ]) | ||
|
|
||
| M = [[0,-1,0], | ||
| [1,0,0]] | ||
|
|
||
| y = affine_transform(x, M, expand=True) | ||
|
|
||
| self.assertTrue(y.shape[0] >= 2 and y.shape[1] >= 2) |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test checks that the shape doesn't shrink when expand=True, but since expand is currently a no-op (as documented in affine.py line 178), this test doesn't actually validate the expand functionality. Consider either skipping this test with a clear comment explaining the feature is not yet implemented, or adding an assertion that the output shape equals the input shape to make the test's purpose clearer.
|
TODOs after today's meeting:
next meeting Tues 15.12. 15:00 with @lolacaro @markak47 @ClaudiaComito |
8a83636 to
d986169
Compare
b0619f6 to
2cda26b
Compare
c0b55a5 to
970c25c
Compare
345ac2c to
2a29fda
Compare
|
**@github-copilot please re-review the PR after the latest changes. |
176c194 to
20ec2d9
Compare
a1731ee to
74f37fa
Compare
|
Thank you for the PR! |
e3d66ad to
144a884
Compare
|
Thank you for the PR! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2048 +/- ##
==========================================
- Coverage 91.96% 91.50% -0.46%
==========================================
Files 88 89 +1
Lines 13496 13660 +164
==========================================
+ Hits 12411 12500 +89
- Misses 1085 1160 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for the PR! |
ClaudiaComito
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-meeting comments for @markak47 , more later!
heat/ndimage/affine.py
Outdated
| if x.split is not None: | ||
| x_full = x.resplit(None) | ||
| y_full = _affine_transform_local(x_full, M, order, mode, constant_value, expand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad because x might be a huge DNDarray that doesn't fit in the RAM of the single process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
- M is 2-D and
- x is 3-D and
- x.split = 0
example: huge stack of images, (1M, 1024, 1024) distributed along the stacking dimension
then _affine_transform_local can be applied to the local slices and resplit(None) is not necessary
same with 3-D M and 4-D x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if x.is_distributed() and x.split != 0:
raise NotImplementedError("Affine transforms along the distributed axis not supported yet")| ht.DNDarray | ||
| Transformed array. | ||
| """ | ||
| M = np.asarray(M) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why numpy?
43a3ebe to
7a6fd1c
Compare
for more information, see https://pre-commit.ci
|
Thank you for the PR! |
DUE DILIGENCE
General
Implementation
(split=None and all valid split axes for 2D and 3D).
DESCRIPTION
This PR adds support for N-dimensional affine transformations to the
heat.ndimage module.
The new implementation supports:
Example scripts are included to demonstrate typical usage on image and
volume data.
Issues resolved: #1900
CHANGES PROPOSED
and sampling
modes, and split configurations
TYPE OF CHANGE
MEMORY REQUIREMENTS
No significant additional memory overhead beyond temporary coordinate
grids required for affine sampling. Memory usage is comparable to existing
ndimage operations in both distributed and non-distributed modes.
PERFORMANCE
Performance is comparable to existing ndimage operations. No performance
regression was observed for standard use cases.
DOES THIS CHANGE MODIFY THE BEHAVIOUR OF OTHER FUNCTIONS?
no