feat: add cropping methods from FMCIB pipeline #113
Conversation
…gger and dictionary iteration
…for quick checking dimension match between image and centroid/mask/dimensions/etc.
WalkthroughThis pull request introduces a module for processing and manipulating images, particularly focused on medical images. It adds several functions for cropping and bounding box operations, as well as centroid calculations. New data classes for representing 3D geometric entities are also created. Additionally, a comprehensive test suite is included to validate the functionality of the cropping methods and bounding box extraction, ensuring robustness and accuracy in image processing tasks. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 48.12% 52.42% +4.29%
==========================================
Files 34 36 +2
Lines 1492 1610 +118
==========================================
+ Hits 718 844 +126
+ Misses 774 766 -8 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/process/images/test_crop.py (1)
4-16: Remove unused imports.Several imported functions from
readii.process.images.cropare not used in the tests:
apply_bounding_box_limitscheck_bounding_box_single_dimensioncrop_to_bounding_boxcrop_to_centroidcrop_to_maxdim_cubefind_bounding_boxfind_centroidresize_imagevalidate_new_dimensions🧰 Tools
🪛 Ruff (0.8.2)
6-6:
readii.process.images.crop.apply_bounding_box_limitsimported but unusedRemove unused import
(F401)
7-7:
readii.process.images.crop.check_bounding_box_single_dimensionimported but unusedRemove unused import
(F401)
9-9:
readii.process.images.crop.crop_to_bounding_boximported but unusedRemove unused import
(F401)
10-10:
readii.process.images.crop.crop_to_centroidimported but unusedRemove unused import
(F401)
11-11:
readii.process.images.crop.crop_to_maxdim_cubeimported but unusedRemove unused import
(F401)
12-12:
readii.process.images.crop.find_bounding_boximported but unusedRemove unused import
(F401)
13-13:
readii.process.images.crop.find_centroidimported but unusedRemove unused import
(F401)
14-14:
readii.process.images.crop.resize_imageimported but unusedRemove unused import
(F401)
15-15:
readii.process.images.crop.validate_new_dimensionsimported but unusedRemove unused import
(F401)
🪛 GitHub Actions: CI-CD
[error] Test failure in test_crop_image_to_mask_methods_complex with parameters [resize_dimensions2-expected_size2-centroid]
src/readii/process/images/crop.py (3)
50-78: Add validation for negative or zero dimensions.The
resize_imagefunction should validate that the resize dimensions are positive numbers to prevent potential issues with negative or zero values.def resize_image(image:sitk.Image, resize_dimensions:tuple ) -> sitk.Image: """Resize an image to specified dimensions via linear interpolation.""" validate_new_dimensions(image, resize_dimensions) + + # Validate positive dimensions + if any(d <= 0 for d in resize_dimensions): + msg = f"Resize dimensions must be positive numbers, got {resize_dimensions}" + logger.exception(msg) + raise ValueError(msg) # Calculate the new spacing based on the resized dimensions
114-117: Consider implementing the bounding box class.The TODO comment suggests moving to a class-based approach for bounding box handling. This would improve code organization and encapsulation.
Would you like me to help design and implement the bounding box class structure?
405-453: Update docstring to match actual method names.The docstring mentions "bounding_box" but the code uses "bbox". Update the documentation to match the actual method names to avoid confusion.
"""Crop an image and mask to an ROI in the mask and resize to a specified crop dimensions. Parameters ---------- crop_method : str, optional - Method to use to crop the image to the mask. Must be one of "bounding_box", "centroid", or "cube". + Method to use to crop the image to the mask. Must be one of "bbox", "centroid", "cube", or "pyradiomics".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/readii/process/images/crop.py(1 hunks)tests/process/images/test_crop.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/process/images/test_crop.py
6-6: readii.process.images.crop.apply_bounding_box_limits imported but unused
Remove unused import
(F401)
7-7: readii.process.images.crop.check_bounding_box_single_dimension imported but unused
Remove unused import
(F401)
9-9: readii.process.images.crop.crop_to_bounding_box imported but unused
Remove unused import
(F401)
10-10: readii.process.images.crop.crop_to_centroid imported but unused
Remove unused import
(F401)
11-11: readii.process.images.crop.crop_to_maxdim_cube imported but unused
Remove unused import
(F401)
12-12: readii.process.images.crop.find_bounding_box imported but unused
Remove unused import
(F401)
13-13: readii.process.images.crop.find_centroid imported but unused
Remove unused import
(F401)
14-14: readii.process.images.crop.resize_image imported but unused
Remove unused import
(F401)
15-15: readii.process.images.crop.validate_new_dimensions imported but unused
Remove unused import
(F401)
🪛 GitHub Actions: CI-CD
tests/process/images/test_crop.py
[error] Test failure in test_crop_image_to_mask_methods_complex with parameters [resize_dimensions2-expected_size2-centroid]
🔇 Additional comments (5)
tests/process/images/test_crop.py (3)
19-53: LGTM! Well-structured fixtures.The fixtures are well-organized and provide the necessary test data for both NSCLC and 4D-Lung datasets.
🧰 Tools
🪛 GitHub Actions: CI-CD
[error] Test failure in test_crop_image_to_mask_methods_complex with parameters [resize_dimensions2-expected_size2-centroid]
82-93: LGTM! Well-designed test fixture.The fixture creates a controlled test environment with specific ROI dimensions (10x20x30) within a larger image (100x100x100), which is excellent for testing edge cases.
🧰 Tools
🪛 GitHub Actions: CI-CD
[error] Test failure in test_crop_image_to_mask_methods_complex with parameters [resize_dimensions2-expected_size2-centroid]
55-80: Verify why PyRadiomics test is commented out.The test case for the "pyradiomics" method is commented out. Please verify if this is intentional or if there are issues with the PyRadiomics implementation that need to be addressed.
✅ Verification successful
PyRadiomics crop test is intentionally commented out due to different output dimensions
The PyRadiomics crop method naturally produces different output dimensions (22,28,14) compared to other methods that use (50,50,50). This appears to be an intentional difference in behavior rather than an issue, as the PyRadiomics functionality is working correctly in production code and is actively maintained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if PyRadiomics is properly installed and configured # Test: Search for PyRadiomics-related issues or TODOs rg -g '!*.pyc' -g '!*venv*' -A 5 'pyradiomics|TODO.*PyRadiomics'Length of output: 50810
Script:
#!/bin/bash # Search for recent changes or issues related to PyRadiomics crop test rg -g '!*.pyc' -g '!*venv*' -A 3 'TODO.*crop|FIXME.*crop|Issue.*crop' # Check git history for relevant commits git log --grep="crop" --grep="pyradiomics" -i --max-count=5 --onelineLength of output: 65608
🧰 Tools
🪛 GitHub Actions: CI-CD
[error] Test failure in test_crop_image_to_mask_methods_complex with parameters [resize_dimensions2-expected_size2-centroid]
src/readii/process/images/crop.py (2)
13-47: LGTM! Well-implemented validation function.The function thoroughly validates dimensions with proper error handling and logging.
365-402: Add tests for PyRadiomics cropping method.The PyRadiomics cropping implementation looks correct, but the test case is commented out. Please add tests to verify this functionality.
…unding box and centroid
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/process/images/test_crop.py (1)
19-53: Consider improving test data management.The test fixtures use hardcoded paths to test data. This approach could make tests brittle and fail in different environments where the data isn't available at the exact paths.
Consider:
- Using a test data configuration file
- Adding checks for data availability
- Using smaller, dedicated test data files instead of full medical images
Example improvement:
import os from pathlib import Path def get_test_data_path(): """Get base path for test data from environment or config.""" return os.getenv('TEST_DATA_PATH', 'tests/data') @pytest.fixture def nsclcCT(): base_path = get_test_data_path() path = Path(base_path) / "NSCLC_Radiogenomics/R01-001/..." if not path.exists(): pytest.skip("Test data not available") return str(path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/process/images/test_crop.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/process/images/test_crop.py
6-6: readii.process.images.crop.apply_bounding_box_limits imported but unused
Remove unused import
(F401)
7-7: readii.process.images.crop.check_bounding_box_single_dimension imported but unused
Remove unused import
(F401)
9-9: readii.process.images.crop.crop_to_bounding_box imported but unused
Remove unused import
(F401)
10-10: readii.process.images.crop.crop_to_centroid imported but unused
Remove unused import
(F401)
11-11: readii.process.images.crop.crop_to_maxdim_cube imported but unused
Remove unused import
(F401)
14-14: readii.process.images.crop.resize_image imported but unused
Remove unused import
(F401)
15-15: readii.process.images.crop.validate_new_dimensions imported but unused
Remove unused import
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-14, py312)
- GitHub Check: Unit-Tests (macos-14, py310)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (macos-latest, py310)
🔇 Additional comments (3)
tests/process/images/test_crop.py (3)
61-61: Uncomment and implement the pyradiomics test case.The pyradiomics test case is commented out. Either implement and uncomment it, or remove it if it's no longer needed.
86-178: Well-structured and comprehensive tests for bounding box and centroid calculations!The test cases cover:
- Simple and complex scenarios
- Edge cases (single-plane ROI, minimum size)
- Error handling (bad labels)
- Clear documentation of test parameters
Great job on the test coverage and organization!
5-16: Verify necessity of imported functions.Several imported functions from
readii.process.images.cropare flagged as unused. While they might be used internally bycrop_image_to_mask, consider removing any that are truly unused to maintain clean imports.Run this script to check if these functions are used internally by
crop_image_to_mask:🧰 Tools
🪛 Ruff (0.8.2)
6-6:
readii.process.images.crop.apply_bounding_box_limitsimported but unusedRemove unused import
(F401)
7-7:
readii.process.images.crop.check_bounding_box_single_dimensionimported but unusedRemove unused import
(F401)
9-9:
readii.process.images.crop.crop_to_bounding_boximported but unusedRemove unused import
(F401)
10-10:
readii.process.images.crop.crop_to_centroidimported but unusedRemove unused import
(F401)
11-11:
readii.process.images.crop.crop_to_maxdim_cubeimported but unusedRemove unused import
(F401)
14-14:
readii.process.images.crop.resize_imageimported but unusedRemove unused import
(F401)
15-15:
readii.process.images.crop.validate_new_dimensionsimported but unusedRemove unused import
(F401)
| def test_crop_image_to_mask_methods( | ||
| lung4D_image, | ||
| lung4D_mask, | ||
| crop_method, | ||
| expected_size, | ||
| resize_dimensions=(50, 50, 50), | ||
| ): | ||
| """Test cropping image to mask with different methods""" | ||
| cropped_image, cropped_mask = crop_image_to_mask( | ||
| lung4D_image, | ||
| lung4D_mask, | ||
| crop_method, | ||
| resize_dimensions, | ||
| ) | ||
| assert ( | ||
| cropped_image.GetSize() == expected_size | ||
| ), f"Cropped image size is incorrect, expected {expected_size}, got {cropped_image.GetSize()}" | ||
| assert ( | ||
| cropped_mask.GetSize() == expected_size | ||
| ), f"Cropped mask size is incorrect, expected {expected_size}, got {cropped_mask.GetSize()}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for cropping methods.
The test only verifies the dimensions of cropped images but doesn't validate:
- The actual content of cropped regions
- Edge cases (empty masks, single-pixel masks)
- Error cases (invalid dimensions)
Consider adding these test cases:
def test_crop_image_to_mask_content():
"""Test that cropped content matches expected regions."""
# Create a test image with known patterns
image = create_test_pattern_image()
mask = create_test_mask()
cropped_image, _ = crop_image_to_mask(image, mask, "bbox")
# Verify specific voxel values in the cropped region
assert cropped_image[25, 25, 25] == expected_value
@pytest.mark.parametrize("invalid_dims", [
(-50, 50, 50),
(0, 0, 0),
(1000, 1000, 1000) # larger than image
])
def test_crop_image_to_mask_invalid_dimensions(invalid_dims):
"""Test handling of invalid dimensions."""
with pytest.raises(ValueError):
crop_image_to_mask(image, mask, "bbox", invalid_dims)
There was a problem hiding this comment.
overall looks good, some general comments
-
It might be hard to test what each step is doing when some functions inevitably resize the results to whats
expected -
wrote some extra tests to just verify that the functions are doing what they expect, commented out the pyradiomics one for now
-
some of my review comments seem nitpicky pointing at type annotations but itll help during overall development if were confident about the granular stuff (cough med-imagetools cough)
keeping readii 2.0 in mind, we should think about how to abstract these where possible, to allow for dependency injection instead of a new function for each crop type
re: the types introduced for Centroid, Coordinate, etc. this is something I am planning to use internally in Med-ImageTools, along with other types. having to unpack tuples can become very error-prone and hard to follow, especially when debugging, and they can make validation of values a lot easier.
| bounding_box = [min_x, max_x, min_y, max_y, min_z, max_z], | ||
| expected_dimensions = crop_dimensions) | ||
|
|
||
| return image[min_x:max_x, min_y:max_y, min_z:max_z] |
There was a problem hiding this comment.
not resizing here, output dimensions would not match the dimensions of the other crops?
There was a problem hiding this comment.
no, because the crop_dimensions are used to make the bounding box starting from the centroid. So it's being cropped to the correct dimension right away.
src/readii/process/images/crop.py
Outdated
| def crop_to_bounding_box(image:sitk.Image, | ||
| bounding_box:tuple[int,int,int,int,int,int], | ||
| resize_dimensions:tuple[int,int,int] | ||
| ) -> sitk.Image: | ||
| """Crop an image to a given bounding box and resize to a specified crop dimensions. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| image : sitk.Image | ||
| Image to crop. | ||
| bounding_box : tuple[int,int,int,int,int,int] | ||
| Bounding box to crop the image to. The order is (min_x, min_y, min_z, max_x, max_y, max_z). | ||
| resize_dimensions : tuple[int,int,int] | ||
| Dimensions to resize the image to. | ||
|
|
||
| Returns | ||
| ------- | ||
| cropped_image : sitk.Image | ||
| Cropped image. | ||
| """ | ||
| # Check that the number of dimensions in the crop dimensions matches the number of dimensions in the image | ||
| validate_new_dimensions(image, resize_dimensions) | ||
|
|
||
| # Check that the number of bounding box dimensions match the image dimensions | ||
| validate_new_dimensions(image, int(len(bounding_box)/2)) | ||
|
|
||
| # Get bounding box dimensions for limit testing | ||
| bounding_box_dimensions = np.array(bounding_box[3:]) - np.array(bounding_box[:3]) | ||
|
|
||
| # Test if bounding box coordinates are within the image, move to image edge if not | ||
| min_x, max_x, min_y, max_y, min_z, max_z = apply_bounding_box_limits(image, bounding_box, bounding_box_dimensions) | ||
|
|
||
| # Crop image to the bounding box | ||
| img_crop = image[min_x:max_x, min_y:max_y, min_z:max_z] | ||
| # Resample the image to the new dimensions and spacing | ||
| img_crop = resize_image(img_crop, resize_dimensions) | ||
| return img_crop | ||
|
|
||
|
|
||
|
|
||
| def crop_to_maxdim_cube(image:sitk.Image, | ||
| bounding_box:tuple[int,int,int,int,int,int], | ||
| resize_dimensions:tuple[int,int,int] | ||
| ) -> sitk.Image: | ||
| """ | ||
| Crop given image to a cube based on the max dim from a bounding box and resize to specified input size. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| image : sitk.Image | ||
| Image to crop. | ||
| bounding_box : tuple[int,int,int,int,int,int] | ||
| Bounding box to find maximum dimension from. The order is (min_x, min_y, min_z, max_x, max_y, max_z). | ||
| resize_dimensions : tuple[int,int,int] | ||
| Crop dimensions to resize the image to. | ||
|
|
||
| Returns | ||
| ------- | ||
| sitk.Image: The cropped and resized image. | ||
| """ | ||
| # Check that the number of dimensions in the crop dimensions matches the number of dimensions in the image | ||
| validate_new_dimensions(image, resize_dimensions) | ||
|
|
||
| # Check that the number of bounding box dimensions match the image dimensions | ||
| validate_new_dimensions(image, len(bounding_box)//2) | ||
|
|
||
| # Extract out the bounding box coordinates | ||
| min_x, max_x, min_y, max_y, min_z, max_z = bounding_box | ||
|
|
||
| # Get maximum dimension of bounding box | ||
| max_dim = max(max_x - min_x, max_y - min_y, max_z - min_z) | ||
| mean_x = int((max_x + min_x) // 2) | ||
| mean_y = int((max_y + min_y) // 2) | ||
| mean_z = int((max_z + min_z) // 2) | ||
|
|
||
| # define new bounding boxes based on the maximum dimension of ROI bounding box | ||
| min_x = int(mean_x - max_dim // 2) | ||
| max_x = int(mean_x + max_dim // 2) | ||
| min_y = int(mean_y - max_dim // 2) | ||
| max_y = int(mean_y + max_dim // 2) | ||
| min_z = int(mean_z - max_dim // 2) | ||
| max_z = int(mean_z + max_dim // 2) | ||
|
|
||
| # Test if bounding box coordinates are within the image, move to image edge if not | ||
| min_x, max_x, min_y, max_y, min_z, max_z = apply_bounding_box_limits(image, | ||
| bounding_box = [min_x, max_x, min_y, max_y, min_z, max_z], | ||
| expected_dimensions = [max_dim, max_dim, max_dim]) | ||
| # Crop image to the cube bounding box | ||
| img_crop = image[min_x:max_x, min_y:max_y, min_z:max_z] | ||
| # Resample the image to the new dimensions and spacing | ||
| img_crop = resize_image(img_crop, resize_dimensions) | ||
| return img_crop | ||
|
|
There was a problem hiding this comment.
this code is a little confusing:
both signatures say that the bounding_box parameter passed in is The order is (min_x, min_y, min_z, max_x, max_y, max_z)
and it looks like bounding_box_dimensions = np.array(bounding_box[3:]) - np.array(bounding_box[:3]) is used to get the sizes in crop_to_bounding_box
but then in the cube one we see min_x, max_x, min_y, max_y, min_z, max_z = bounding_box
the find_bounding_box function used to get the bounding boxes returns:
xstart, xend, ystart, yend, zstart, zend
…adiomics doesn't require it
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/readii/process/images/__init__.py(1 hunks)src/readii/process/images/crop.py(1 hunks)tests/process/images/test_crop.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/readii/process/images/init.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/process/images/test_crop.py
6-6: readii.process.images.crop.apply_bounding_box_limits imported but unused
Remove unused import
(F401)
7-7: readii.process.images.crop.check_bounding_box_single_dimension imported but unused
Remove unused import
(F401)
9-9: readii.process.images.crop.crop_to_bounding_box imported but unused
Remove unused import
(F401)
10-10: readii.process.images.crop.crop_to_centroid imported but unused
Remove unused import
(F401)
11-11: readii.process.images.crop.crop_to_maxdim_cube imported but unused
Remove unused import
(F401)
14-14: readii.process.images.crop.validate_new_dimensions imported but unused
Remove unused import
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-14, py312)
- GitHub Check: Unit-Tests (macos-14, py311)
- GitHub Check: Unit-Tests (macos-14, py310)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (macos-latest, py310)
🔇 Additional comments (7)
tests/process/images/test_crop.py (3)
111-119: LGTM!The test properly verifies error handling for invalid labels in both
find_bounding_boxandfind_centroidfunctions.
125-152: LGTM!The test provides good coverage for bounding box calculation with various ROI configurations, including edge cases.
158-177: LGTM!The test provides good coverage for centroid calculation with various ROI configurations, including edge cases.
src/readii/process/images/crop.py (4)
13-46: LGTM!The function is well-implemented with proper type checking using
isinstance(), error handling, and logging.
90-133: LGTM!The function is well-implemented with proper error handling and includes clear examples in the docstring.
197-235:⚠️ Potential issueAdd handling for large resize dimensions.
The function fails when crop dimensions are larger than the image dimensions.
Apply this diff to add handling for large resize dimensions:
validate_new_dimensions(image, centroid) + # Get image dimensions + img_size = image.GetSize() + + # If any crop dimension is larger than image dimension, return resized full image + if any(c > i for c, i in zip(crop_dimensions, img_size)): + return resize(image, size=crop_dimensions) + min_x = int(centroid[0] - crop_dimensions[0] // 2)Likely invalid or redundant comment.
50-87:⚠️ Potential issueFix docstring return type.
The docstring indicates that the function returns a
np.ndarray, but it actually returns atuple.Apply this diff to fix the docstring:
Returns ------- - bounding_box : np.ndarray - Numpy array containing the bounding box coordinates around the ROI. + bounding_box : tuple + Tuple containing the bounding box coordinates (xstart, xend, ystart, yend, zstart, zend).Likely invalid or redundant comment.
tests/process/images/test_crop.py
Outdated
| @pytest.mark.parametrize( | ||
| "crop_method, expected_size", | ||
| [ | ||
| ("bbox", (50, 50, 50)), | ||
| ("centroid", (50, 50, 50)), | ||
| ("cube", (50, 50, 50)), | ||
| # ("pyradiomics", (22, 28, 14)), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for cropping methods.
The test only verifies the dimensions of cropped images but doesn't validate the actual content of cropped regions.
Add test cases to verify that the cropped content matches expected regions:
@pytest.mark.parametrize(
"crop_method, expected_size, expected_content",
[
("bbox", (50, 50, 50), [(25, 25, 25, 100)]), # x, y, z, expected_value
("centroid", (50, 50, 50), [(25, 25, 25, 100)]),
("cube", (50, 50, 50), [(25, 25, 25, 100)]),
],
)
def test_crop_image_to_mask_content(lung4D_image, lung4D_mask, crop_method, expected_size, expected_content):
"""Test that cropped content matches expected regions."""
cropped_image, _ = crop_image_to_mask(lung4D_image, lung4D_mask, crop_method, expected_size)
for x, y, z, expected_value in expected_content:
assert cropped_image[x, y, z] == expected_value| def test_crop_image_to_mask_methods( | ||
| lung4D_image, | ||
| lung4D_mask, | ||
| crop_method, | ||
| expected_size, | ||
| resize_dimensions=(50, 50, 50), | ||
| ): | ||
| """Test cropping image to mask with different methods""" | ||
| cropped_image, cropped_mask = crop_image_to_mask( | ||
| lung4D_image, | ||
| lung4D_mask, | ||
| crop_method, | ||
| resize_dimensions, | ||
| ) | ||
| assert ( | ||
| cropped_image.GetSize() == expected_size | ||
| ), f"Cropped image size is incorrect, expected {expected_size}, got {cropped_image.GetSize()}" | ||
| assert ( | ||
| cropped_mask.GetSize() == expected_size | ||
| ), f"Cropped mask size is incorrect, expected {expected_size}, got {cropped_mask.GetSize()}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for error handling.
The test doesn't verify error handling for invalid inputs.
Add test cases to verify error handling:
@pytest.mark.parametrize(
"crop_method, resize_dimensions, expected_error",
[
("invalid_method", (50, 50, 50), ValueError),
("bbox", (-50, 50, 50), ValueError),
("centroid", (0, 0, 0), ValueError),
("cube", (1000, 1000, 1000), ValueError),
],
)
def test_crop_image_to_mask_error_handling(lung4D_image, lung4D_mask, crop_method, resize_dimensions, expected_error):
"""Test error handling for invalid inputs."""
with pytest.raises(expected_error):
crop_image_to_mask(lung4D_image, lung4D_mask, crop_method, resize_dimensions)
src/readii/process/images/crop.py
Outdated
| def find_centroid(mask:sitk.Image) -> np.ndarray: | ||
| """Find the centroid of a region of interest (ROI) in a given binary mask image. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| mask : sitk.Image | ||
| Mask image to find the centroid within. | ||
|
|
||
| Returns | ||
| ------- | ||
| centroid : np.ndarray | ||
| Numpy array containing the coordinates of the ROI centroid. | ||
| """ | ||
| # Convert the mask to a uint8 image | ||
| mask_uint = sitk.Cast(mask, sitk.sitkUInt8) | ||
| stats = sitk.LabelShapeStatisticsImageFilter() | ||
| stats.Execute(mask_uint) | ||
| # Get the centroid coordinates as a physical point in the mask | ||
| centroid_coords = stats.GetCentroid(1) | ||
| # Convert the physical point to an index in the mask array | ||
| centroid_idx = mask.TransformPhysicalPointToIndex(centroid_coords) | ||
| return centroid_idx |
There was a problem hiding this comment.
Fix docstring return type.
The docstring indicates that the function returns a np.ndarray, but it actually returns a tuple.
Apply this diff to fix the docstring:
Returns
-------
- centroid : np.ndarray
- Numpy array containing the coordinates of the ROI centroid.
+ centroid : tuple
+ Tuple containing the coordinates of the ROI centroid.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def find_centroid(mask:sitk.Image) -> np.ndarray: | |
| """Find the centroid of a region of interest (ROI) in a given binary mask image. | |
| Parameters | |
| ---------- | |
| mask : sitk.Image | |
| Mask image to find the centroid within. | |
| Returns | |
| ------- | |
| centroid : np.ndarray | |
| Numpy array containing the coordinates of the ROI centroid. | |
| """ | |
| # Convert the mask to a uint8 image | |
| mask_uint = sitk.Cast(mask, sitk.sitkUInt8) | |
| stats = sitk.LabelShapeStatisticsImageFilter() | |
| stats.Execute(mask_uint) | |
| # Get the centroid coordinates as a physical point in the mask | |
| centroid_coords = stats.GetCentroid(1) | |
| # Convert the physical point to an index in the mask array | |
| centroid_idx = mask.TransformPhysicalPointToIndex(centroid_coords) | |
| return centroid_idx | |
| def find_centroid(mask:sitk.Image) -> np.ndarray: | |
| """Find the centroid of a region of interest (ROI) in a given binary mask image. | |
| Parameters | |
| ---------- | |
| mask : sitk.Image | |
| Mask image to find the centroid within. | |
| Returns | |
| ------- | |
| centroid : tuple | |
| Tuple containing the coordinates of the ROI centroid. | |
| """ | |
| # Convert the mask to a uint8 image | |
| mask_uint = sitk.Cast(mask, sitk.sitkUInt8) | |
| stats = sitk.LabelShapeStatisticsImageFilter() | |
| stats.Execute(mask_uint) | |
| # Get the centroid coordinates as a physical point in the mask | |
| centroid_coords = stats.GetCentroid(1) | |
| # Convert the physical point to an index in the mask array | |
| centroid_idx = mask.TransformPhysicalPointToIndex(centroid_coords) | |
| return centroid_idx |
src/readii/process/images/crop.py
Outdated
| def crop_to_bounding_box(image:sitk.Image, | ||
| bounding_box:tuple[int,int,int,int,int,int], | ||
| resize_dimensions:tuple[int,int,int] | ||
| ) -> sitk.Image: | ||
| """Crop an image to a given bounding box and resize to a specified crop dimensions. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| image : sitk.Image | ||
| Image to crop. | ||
| bounding_box : tuple[int,int,int,int,int,int] | ||
| Bounding box to crop the image to. The order is (min_x, min_y, min_z, max_x, max_y, max_z). | ||
| resize_dimensions : tuple[int,int,int] | ||
| Dimensions to resize the image to. | ||
|
|
||
| Returns | ||
| ------- | ||
| cropped_image : sitk.Image | ||
| Cropped image. | ||
| """ | ||
| # Check that the number of dimensions in the crop dimensions matches the number of dimensions in the image | ||
| validate_new_dimensions(image, resize_dimensions) | ||
|
|
||
| # Check that the number of bounding box dimensions match the image dimensions | ||
| validate_new_dimensions(image, int(len(bounding_box)/2)) | ||
|
|
||
| # Get bounding box dimensions for limit testing | ||
| bounding_box_dimensions = np.array(bounding_box[3:]) - np.array(bounding_box[:3]) | ||
|
|
||
| # Test if bounding box coordinates are within the image, move to image edge if not | ||
| min_x, max_x, min_y, max_y, min_z, max_z = apply_bounding_box_limits(image, bounding_box, bounding_box_dimensions) | ||
|
|
||
| # Crop image to the bounding box | ||
| img_crop = image[min_x:max_x, min_y:max_y, min_z:max_z] | ||
| # Resample the image to the new dimensions and spacing | ||
| img_crop = resize(img_crop, size = resize_dimensions) | ||
| return img_crop |
There was a problem hiding this comment.
Fix docstring parameter description.
The docstring for bounding_box parameter doesn't match the actual order of coordinates.
Apply this diff to fix the docstring:
bounding_box : tuple[int,int,int,int,int,int]
- Bounding box to crop the image to. The order is (min_x, min_y, min_z, max_x, max_y, max_z).
+ Bounding box to crop the image to. The order is (min_x, max_x, min_y, max_y, min_z, max_z).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def crop_to_bounding_box(image:sitk.Image, | |
| bounding_box:tuple[int,int,int,int,int,int], | |
| resize_dimensions:tuple[int,int,int] | |
| ) -> sitk.Image: | |
| """Crop an image to a given bounding box and resize to a specified crop dimensions. | |
| Parameters | |
| ---------- | |
| image : sitk.Image | |
| Image to crop. | |
| bounding_box : tuple[int,int,int,int,int,int] | |
| Bounding box to crop the image to. The order is (min_x, min_y, min_z, max_x, max_y, max_z). | |
| resize_dimensions : tuple[int,int,int] | |
| Dimensions to resize the image to. | |
| Returns | |
| ------- | |
| cropped_image : sitk.Image | |
| Cropped image. | |
| """ | |
| # Check that the number of dimensions in the crop dimensions matches the number of dimensions in the image | |
| validate_new_dimensions(image, resize_dimensions) | |
| # Check that the number of bounding box dimensions match the image dimensions | |
| validate_new_dimensions(image, int(len(bounding_box)/2)) | |
| # Get bounding box dimensions for limit testing | |
| bounding_box_dimensions = np.array(bounding_box[3:]) - np.array(bounding_box[:3]) | |
| # Test if bounding box coordinates are within the image, move to image edge if not | |
| min_x, max_x, min_y, max_y, min_z, max_z = apply_bounding_box_limits(image, bounding_box, bounding_box_dimensions) | |
| # Crop image to the bounding box | |
| img_crop = image[min_x:max_x, min_y:max_y, min_z:max_z] | |
| # Resample the image to the new dimensions and spacing | |
| img_crop = resize(img_crop, size = resize_dimensions) | |
| return img_crop | |
| def crop_to_bounding_box(image:sitk.Image, | |
| bounding_box:tuple[int,int,int,int,int,int], | |
| resize_dimensions:tuple[int,int,int] | |
| ) -> sitk.Image: | |
| """Crop an image to a given bounding box and resize to a specified crop dimensions. | |
| Parameters | |
| ---------- | |
| image : sitk.Image | |
| Image to crop. | |
| bounding_box : tuple[int,int,int,int,int,int] | |
| Bounding box to crop the image to. The order is (min_x, max_x, min_y, max_y, min_z, max_z). | |
| resize_dimensions : tuple[int,int,int] | |
| Dimensions to resize the image to. | |
| Returns | |
| ------- | |
| cropped_image : sitk.Image | |
| Cropped image. | |
| """ | |
| # Check that the number of dimensions in the crop dimensions matches the number of dimensions in the image | |
| validate_new_dimensions(image, resize_dimensions) | |
| # Check that the number of bounding box dimensions match the image dimensions | |
| validate_new_dimensions(image, int(len(bounding_box)/2)) | |
| # Get bounding box dimensions for limit testing | |
| bounding_box_dimensions = np.array(bounding_box[3:]) - np.array(bounding_box[:3]) | |
| # Test if bounding box coordinates are within the image, move to image edge if not | |
| min_x, max_x, min_y, max_y, min_z, max_z = apply_bounding_box_limits(image, bounding_box, bounding_box_dimensions) | |
| # Crop image to the bounding box | |
| img_crop = image[min_x:max_x, min_y:max_y, min_z:max_z] | |
| # Resample the image to the new dimensions and spacing | |
| img_crop = resize(img_crop, size = resize_dimensions) | |
| return img_crop |
src/readii/process/images/crop.py
Outdated
| def apply_bounding_box_limits(image:sitk.Image, | ||
| bounding_box:tuple[int,int,int,int,int,int], | ||
| expected_dimensions:tuple[int,int,int] | ||
| ) -> tuple: | ||
| """Check that bounding box coordinates are within the image dimensions. If not, move bounding box to the edge of the image and expand to expected dimension. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| image : sitk.Image | ||
| Image to check the bounding box coordinates against. | ||
| bounding_box : tuple[int,int,int,int,int,int] | ||
| Bounding box to check the coordinates of. | ||
| expected_dimensions : tuple[int,int,int] | ||
| Expected dimensions of the bounding box. Used if the bounding box needs to be shifted to the edge of the image. | ||
|
|
||
| Returns | ||
| ------- | ||
| min_x, min_y, min_z, max_x, max_y, max_z : tuple[int,int,int,int,int,int] | ||
| Updated bounding box coordinates. | ||
| """ | ||
| # Get the size of the image to use to determine if crop dimensions are larger than the image | ||
| img_x, img_y, img_z = image.GetSize() | ||
|
|
||
| # Extract the bounding box coordinates | ||
| min_x, max_x, min_y, max_y, min_z, max_z = bounding_box | ||
|
|
||
| # Check each bounding box dimensions coordinates and move to image edge if not within image | ||
| min_x, max_x = check_bounding_box_single_dimension(min_x, max_x, expected_dimensions[0], img_x) | ||
| min_y, max_y = check_bounding_box_single_dimension(min_y, max_y, expected_dimensions[1], img_y) | ||
| min_z, max_z = check_bounding_box_single_dimension(min_z, max_z, expected_dimensions[2], img_z) | ||
|
|
||
| return min_x, max_x, min_y, max_y, min_z, max_z |
There was a problem hiding this comment.
Fix docstring parameter description.
The docstring for bounding_box parameter doesn't match the actual order of coordinates.
Apply this diff to fix the docstring:
bounding_box : tuple[int,int,int,int,int,int]
- Bounding box to check the coordinates of.
+ Bounding box to check the coordinates of. The order is (min_x, max_x, min_y, max_y, min_z, max_z).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def apply_bounding_box_limits(image:sitk.Image, | |
| bounding_box:tuple[int,int,int,int,int,int], | |
| expected_dimensions:tuple[int,int,int] | |
| ) -> tuple: | |
| """Check that bounding box coordinates are within the image dimensions. If not, move bounding box to the edge of the image and expand to expected dimension. | |
| Parameters | |
| ---------- | |
| image : sitk.Image | |
| Image to check the bounding box coordinates against. | |
| bounding_box : tuple[int,int,int,int,int,int] | |
| Bounding box to check the coordinates of. | |
| expected_dimensions : tuple[int,int,int] | |
| Expected dimensions of the bounding box. Used if the bounding box needs to be shifted to the edge of the image. | |
| Returns | |
| ------- | |
| min_x, min_y, min_z, max_x, max_y, max_z : tuple[int,int,int,int,int,int] | |
| Updated bounding box coordinates. | |
| """ | |
| # Get the size of the image to use to determine if crop dimensions are larger than the image | |
| img_x, img_y, img_z = image.GetSize() | |
| # Extract the bounding box coordinates | |
| min_x, max_x, min_y, max_y, min_z, max_z = bounding_box | |
| # Check each bounding box dimensions coordinates and move to image edge if not within image | |
| min_x, max_x = check_bounding_box_single_dimension(min_x, max_x, expected_dimensions[0], img_x) | |
| min_y, max_y = check_bounding_box_single_dimension(min_y, max_y, expected_dimensions[1], img_y) | |
| min_z, max_z = check_bounding_box_single_dimension(min_z, max_z, expected_dimensions[2], img_z) | |
| return min_x, max_x, min_y, max_y, min_z, max_z | |
| def apply_bounding_box_limits(image:sitk.Image, | |
| bounding_box:tuple[int,int,int,int,int,int], | |
| expected_dimensions:tuple[int,int,int] | |
| ) -> tuple: | |
| """Check that bounding box coordinates are within the image dimensions. If not, move bounding box to the edge of the image and expand to expected dimension. | |
| Parameters | |
| ---------- | |
| image : sitk.Image | |
| Image to check the bounding box coordinates against. | |
| bounding_box : tuple[int,int,int,int,int,int] | |
| Bounding box to check the coordinates of. The order is (min_x, max_x, min_y, max_y, min_z, max_z). | |
| expected_dimensions : tuple[int,int,int] | |
| Expected dimensions of the bounding box. Used if the bounding box needs to be shifted to the edge of the image. | |
| Returns | |
| ------- | |
| min_x, min_y, min_z, max_x, max_y, max_z : tuple[int,int,int,int,int,int] | |
| Updated bounding box coordinates. | |
| """ | |
| # Get the size of the image to use to determine if crop dimensions are larger than the image | |
| img_x, img_y, img_z = image.GetSize() | |
| # Extract the bounding box coordinates | |
| min_x, max_x, min_y, max_y, min_z, max_z = bounding_box | |
| # Check each bounding box dimensions coordinates and move to image edge if not within image | |
| min_x, max_x = check_bounding_box_single_dimension(min_x, max_x, expected_dimensions[0], img_x) | |
| min_y, max_y = check_bounding_box_single_dimension(min_y, max_y, expected_dimensions[1], img_y) | |
| min_z, max_z = check_bounding_box_single_dimension(min_z, max_z, expected_dimensions[2], img_z) | |
| return min_x, max_x, min_y, max_y, min_z, max_z |
|
So I was just pretty much copying what was working in the FMCIB pipeline and then planning to make it better as we worked on READII 2.0, so that kind of answers most of these but figured I'd address these now.
Yeah, I've already noticed this as I started writing test functions.
Agreed, I've been using ruff pretty regularly to try and catch them all, but appreciate the extra check.
I would like to organize these like the negative controls, just didn't have the brain power to do that the other day.
I totally agree. I just got really stuck trying to mix between my FMCIB code I knew worked and figuring out how to use those objects. Had to commit to one of them. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/readii/process/images/crop.py (1)
83-85: Consider implementing the suggested bounding box class.The TODO comment suggests using a class for bounding box coordinates, which would improve type safety and make the code more maintainable. This would help avoid coordinate order confusion and make the code more self-documenting.
Would you like me to help design the
BoundingBoxclass with proper coordinate handling?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/readii/process/images/crop.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/readii/process/images/crop.py
268-268: Undefined name max_x
(F821)
268-268: Undefined name min_x
(F821)
268-268: Undefined name max_y
(F821)
268-268: Undefined name min_y
(F821)
268-268: Undefined name max_z
(F821)
268-268: Undefined name min_z
(F821)
398-398: Use != to compare constant literals
Replace is not with !=
(F632)
🪛 GitHub Actions: CI-CD
src/readii/process/images/crop.py
[error] 268-268: UnboundLocalError: Cannot access local variable 'max_x' where it is not associated with a value in apply_bounding_box_limits function
[warning] 398-398: SyntaxWarning: 'is not' with a literal. Did you mean '!='? in condition 'crop_method is not "pyradiomics"'
🔇 Additional comments (1)
src/readii/process/images/crop.py (1)
172-193: Fix docstring return type.The docstring indicates that the function returns a
np.ndarray, but it actually returns atuple.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/readii/process/images/crop.py (3)
13-47: Consider consolidating error logging.The error logging pattern is repeated multiple times. Consider creating a helper function to handle error logging and raising.
+def _raise_dimension_error(msg: str) -> None: + logger.exception(msg) + raise ValueError(msg) def validate_new_dimensions(image:sitk.Image, new_dimensions:tuple | int ) -> None: if isinstance(new_dimensions, tuple): if len(new_dimensions) != image.GetDimension(): msg = f"Number of dimensions in new_dimensions ({len(new_dimensions)}) does not match the number of dimensions in the image ({image.GetDimension()})." - logger.exception(msg) - raise ValueError(msg) + _raise_dimension_error(msg)
83-85: Consider implementing the suggested class structure.The TODO comment suggests using a class for the bounding box, which would improve type safety and make the code more maintainable.
Would you like me to help design the
CoordinateandBoundingBoxclasses?
365-368: Improve mask correction logic.The condition for updating the mask could be more explicit.
- if corrected_mask: + if corrected_mask is not None: mask = corrected_mask
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/readii/process/images/crop.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/readii/process/images/crop.py
266-266: Undefined name max_x
(F821)
266-266: Undefined name min_x
(F821)
266-266: Undefined name max_y
(F821)
266-266: Undefined name min_y
(F821)
266-266: Undefined name max_z
(F821)
266-266: Undefined name min_z
(F821)
🪛 GitHub Actions: CI-CD
src/readii/process/images/crop.py
[error] 426-426: Invalid crop method handling: The code expects 'bounding_box' but test uses 'bbox'. Method name mismatch in crop_image_to_mask function.
🔇 Additional comments (2)
src/readii/process/images/crop.py (2)
1-11: LGTM! Well-organized imports with proper type hints.The imports are logically grouped and all necessary for the functionality.
401-403:⚠️ Potential issueFix string comparison operator.
Using
is notwith string literals is incorrect and could lead to unexpected behavior.- if resize_dimensions is None and crop_method is not "pyradiomics": + if resize_dimensions is None and crop_method != "pyradiomics":Likely invalid or redundant comment.
|
|
||
| def crop_image_to_mask(image:sitk.Image, | ||
| mask:sitk.Image, | ||
| crop_method:Literal["bounding_box", "centroid", "cube", "pyradiomics"], |
There was a problem hiding this comment.
Fix inconsistent method names.
The code uses "bounding_box" but the error message uses "bbox", causing confusion and test failures.
- crop_method:Literal["bounding_box", "centroid", "cube", "pyradiomics"],
+ crop_method:Literal["bbox", "centroid", "cube", "pyradiomics"],
- case "bounding_box":
+ case "bbox":
- msg = f"Invalid crop method: {crop_method}. Must be one of 'bbox', 'centroid', 'cube', or 'pyradiomics'."
+ msg = f"Invalid crop method: {crop_method}. Must be one of 'bbox', 'centroid', 'cube', or 'pyradiomics'."Also applies to: 406-406, 425-425
src/readii/process/images/crop.py
Outdated
| bounding_box : np.ndarray | ||
| Numpy array containing the bounding box coordinates around the ROI. Order is [min_x, max_x, min_y, max_y, min_z, max_z]. |
There was a problem hiding this comment.
Fix return type annotation mismatch.
The docstring specifies np.ndarray but the function's return type hint is tuple.
-def find_bounding_box(mask:sitk.Image,
- min_dim_size:int = 4
- ) -> tuple:
+def find_bounding_box(mask:sitk.Image,
+ min_dim_size:int = 4
+ ) -> tuple[int, int, int, int, int, int]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bounding_box : np.ndarray | |
| Numpy array containing the bounding box coordinates around the ROI. Order is [min_x, max_x, min_y, max_y, min_z, max_z]. | |
| def find_bounding_box(mask:sitk.Image, | |
| min_dim_size:int = 4 | |
| ) -> tuple[int, int, int, int, int, int]: |
src/readii/process/images/crop.py
Outdated
| # Current bounding box dimensions | ||
| current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z) | ||
|
|
There was a problem hiding this comment.
Fix undefined variables usage.
The variables min_x, max_x, etc. are used before they're extracted from the bounding box.
- # Current bounding box dimensions
- current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z)
+ # Extract the bounding box coordinates first
+ min_x, max_x, min_y, max_y, min_z, max_z = bounding_box
+
+ # Current bounding box dimensions
+ current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Current bounding box dimensions | |
| current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z) | |
| # Extract the bounding box coordinates first | |
| min_x, max_x, min_y, max_y, min_z, max_z = bounding_box | |
| # Current bounding box dimensions | |
| current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z) | |
🧰 Tools
🪛 Ruff (0.8.2)
266-266: Undefined name max_x
(F821)
266-266: Undefined name min_x
(F821)
266-266: Undefined name max_y
(F821)
266-266: Undefined name min_y
(F821)
266-266: Undefined name max_z
(F821)
266-266: Undefined name min_z
(F821)
…te and Size3D objects from utils
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/readii/process/images/utils/bounding_box.py (2)
27-31: Consider adding dimension validation.Since this class represents dimensions, consider adding validation to ensure values are positive.
@dataclass class Size3D(Point3D): """Represent the size of a 3D object using its width, height, and depth.""" - pass + def __post_init__(self): + """Validate that dimensions are positive.""" + if any(dim < 0 for dim in (self.x, self.y, self.z)): + raise ValueError("Dimensions must be positive")
34-44: Consider adding bounds validation.Since this represents a coordinate in 3D space, consider adding validation to ensure coordinates are within valid bounds when performing operations.
def __add__(self, other: Size3D) -> Coordinate: """Add a size to a coordinate to get a second coordinate.""" + # Optional: Add validation for maximum bounds + max_bounds = (1000, 1000, 1000) # Example bounds + result = Coordinate(x=self.x + other.x, y=self.y + other.y, z=self.z + other.z) + if any(coord > bound for coord, bound in zip((result.x, result.y, result.z), max_bounds)): + raise ValueError("Resulting coordinate exceeds maximum bounds") - return Coordinate(x=self.x + other.x, y=self.y + other.y, z=self.z + other.z) + return resulttests/process/images/test_crop.py (2)
18-51: Consider using pathlib for test paths.Using pathlib would make the path handling more robust across different operating systems.
+from pathlib import Path @pytest.fixture def nsclcCT(): - return "tests/NSCLC_Radiogenomics/R01-001/09-06-1990-NA-CT_CHEST_ABD_PELVIS_WITH_CON-98785/3.000000-THORAX_1.0_B45f-95741" + return Path("tests/NSCLC_Radiogenomics/R01-001/09-06-1990-NA-CT_CHEST_ABD_PELVIS_WITH_CON-98785/3.000000-THORAX_1.0_B45f-95741")
125-177: LGTM! Well-structured test cases.The parameterized tests cover a good range of scenarios. Consider adding edge cases:
- Empty mask (all zeros)
- Full mask (all ones)
- Mask with multiple disconnected regions
@pytest.mark.parametrize( "image_and_mask_with_roi, expected_bbox", [ # Empty mask ((0, 0, 0, 0, 0, 0), (0, 4, 0, 4, 0, 4)), # Full mask ((0, 100, 0, 100, 0, 100), (0, 100, 0, 100, 0, 100)), ], indirect=["image_and_mask_with_roi"], )src/readii/process/images/crop.py (1)
364-365: Use explicit None check for mask_label.The condition could be more explicit to handle edge cases better.
- if mask_label is None: + if mask_label is None: + # Log that we're using default label + logger.info("No mask label provided, using first non-zero voxel label")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
notebooks/crop_testing.ipynb(1 hunks)notebooks/viz_neg_controls.ipynb(4 hunks)src/readii/process/images/crop.py(1 hunks)src/readii/process/images/utils/bounding_box.py(1 hunks)tests/process/images/test_crop.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/readii/process/images/crop.py
270-270: Undefined name max_x
(F821)
270-270: Undefined name min_x
(F821)
270-270: Undefined name max_y
(F821)
270-270: Undefined name min_y
(F821)
270-270: Undefined name max_z
(F821)
270-270: Undefined name min_z
(F821)
tests/process/images/test_crop.py
6-6: readii.process.images.crop.apply_bounding_box_limits imported but unused
Remove unused import
(F401)
7-7: readii.process.images.crop.check_bounding_box_single_dimension imported but unused
Remove unused import
(F401)
9-9: readii.process.images.crop.crop_to_bounding_box imported but unused
Remove unused import
(F401)
10-10: readii.process.images.crop.crop_to_centroid imported but unused
Remove unused import
(F401)
11-11: readii.process.images.crop.crop_to_maxdim_cube imported but unused
Remove unused import
(F401)
14-14: readii.process.images.crop.validate_new_dimensions imported but unused
Remove unused import
(F401)
🔇 Additional comments (7)
notebooks/viz_neg_controls.ipynb (2)
102-108: Great improvements to the visualization!The changes enhance the visual presentation by:
- Using appropriate color maps ('grey' for the donut image and 'Greens' for the mask)
- Removing axis ticks for a cleaner look
388-402: Environment configuration looks good.The kernel name and Python version updates maintain consistency with the development environment.
src/readii/process/images/utils/bounding_box.py (2)
6-24: LGTM! Clean implementation of the base Point3D class.The implementation is well-structured with proper type hints and clear docstrings. The use of @DataClass reduces boilerplate code.
48-62: LGTM! Well-documented centroid class.The docstring provides clear and detailed information about the class purpose and attributes.
src/readii/process/images/crop.py (3)
14-47: LGTM! Robust dimension validation.The function properly validates dimensions with clear error messages and good type checking.
406-408:⚠️ Potential issueFix string comparison operator.
Using
is notwith string literals is incorrect. Use!=instead.- if resize_dimensions is None and crop_method is not "pyradiomics": + if resize_dimensions is None and crop_method != "pyradiomics":Likely invalid or redundant comment.
381-431:⚠️ Potential issueFix inconsistent method names.
The code uses "bounding_box" in the Literal type but "bbox" elsewhere. This inconsistency could cause errors.
- crop_method:Literal["bounding_box", "centroid", "cube", "pyradiomics"], + crop_method:Literal["bbox", "centroid", "cube", "pyradiomics"], - case "bounding_box": + case "bbox": - msg = f"Invalid crop method: {crop_method}. Must be one of 'bounding_box', 'centroid', 'cube', or 'pyradiomics'." + msg = f"Invalid crop method: {crop_method}. Must be one of 'bbox', 'centroid', 'cube', or 'pyradiomics'."Likely invalid or redundant comment.
| @pytest.mark.parametrize( | ||
| "crop_method, expected_size", | ||
| [ | ||
| ("bounding_box", (50, 50, 50)), | ||
| ("centroid", (50, 50, 50)), | ||
| ("cube", (50, 50, 50)), | ||
| # ("pyradiomics", (22, 28, 14)), | ||
| ], | ||
| ) | ||
| def test_crop_image_to_mask_methods( | ||
| lung4D_image, | ||
| lung4D_mask, | ||
| crop_method, | ||
| expected_size, | ||
| resize_dimensions=(50, 50, 50), | ||
| ): | ||
| """Test cropping image to mask with different methods""" | ||
| cropped_image, cropped_mask = crop_image_to_mask( | ||
| lung4D_image, | ||
| lung4D_mask, | ||
| crop_method, | ||
| resize_dimensions, | ||
| ) | ||
| assert ( | ||
| cropped_image.GetSize() == expected_size | ||
| ), f"Cropped image size is incorrect, expected {expected_size}, got {cropped_image.GetSize()}" | ||
| assert ( | ||
| cropped_mask.GetSize() == expected_size | ||
| ), f"Cropped mask size is incorrect, expected {expected_size}, got {cropped_mask.GetSize()}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for cropping methods.
The test only verifies dimensions but should also validate:
- The actual content of cropped regions
- Edge cases (empty masks, single-pixel masks)
- Error cases (invalid dimensions)
@pytest.mark.parametrize("invalid_input", [
(None, None),
(sitk.Image(1,1,1), None),
(None, sitk.Image(1,1,1))
])
def test_crop_image_to_mask_invalid_input(invalid_input):
"""Test error handling for invalid inputs."""
image, mask = invalid_input
with pytest.raises(ValueError):
crop_image_to_mask(image, mask, "bbox", (50,50,50))| "source": [ | ||
| "centroid = Centroid(centroid_x, centroid_y, centroid_z)\n", | ||
| "crop_dimensions = Size3D(50,50,50)\n", | ||
| "\n", | ||
| "centroid - crop_dimensions " |
There was a problem hiding this comment.
Fix error in centroid calculation.
The notebook shows an AttributeError in the centroid calculation. This needs to be fixed before merging.
-centroid - crop_dimensions
+# First ensure the centroid coordinates are valid
+if not all(0 <= c < s for c, s in zip(centroid.as_tuple, image.GetSize())):
+ raise ValueError("Centroid coordinates must be within image dimensions")
+# Then perform the calculation
+result = centroid - crop_dimensions📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "source": [ | |
| "centroid = Centroid(centroid_x, centroid_y, centroid_z)\n", | |
| "crop_dimensions = Size3D(50,50,50)\n", | |
| "\n", | |
| "centroid - crop_dimensions " | |
| "source": [ | |
| "centroid = Centroid(centroid_x, centroid_y, centroid_z)\n", | |
| "crop_dimensions = Size3D(50,50,50)\n", | |
| "\n", | |
| "# First ensure the centroid coordinates are valid\n", | |
| "if not all(0 <= c < s for c, s in zip(centroid.as_tuple, image.GetSize())):\n", | |
| " raise ValueError(\"Centroid coordinates must be within image dimensions\")\n", | |
| "# Then perform the calculation\n", | |
| "result = centroid - crop_dimensions" | |
| ] |
| "ct_path = \"/Users/katyscott/Documents/READII-2-ROQC/readii-fmcib/archive/temp_complete_output/procdata/RADCURE/images/mit_outputs/0_RADCURE-0020/CT/CT.nii.gz\"\n", | ||
| "mask_path = \"/Users/katyscott/Documents/READII-2-ROQC/readii-fmcib/archive/temp_complete_output/procdata/RADCURE/images/mit_outputs/0_RADCURE-0020/RTSTRUCT_CT/GTV.nii.gz\"\n", |
There was a problem hiding this comment.
Remove hardcoded paths containing sensitive information.
The paths contain usernames and potentially sensitive directory structures. Consider:
- Using environment variables or config files
- Using relative paths
- Removing sensitive information
-ct_path = "/Users/katyscott/Documents/READII-2-ROQC/readii-fmcib/archive/temp_complete_output/procdata/RADCURE/images/mit_outputs/0_RADCURE-0020/CT/CT.nii.gz"
-mask_path = "/Users/katyscott/Documents/READII-2-ROQC/readii-fmcib/archive/temp_complete_output/procdata/RADCURE/images/mit_outputs/0_RADCURE-0020/RTSTRUCT_CT/GTV.nii.gz"
+from pathlib import Path
+ct_path = Path("data/test/CT.nii.gz")
+mask_path = Path("data/test/GTV.nii.gz")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ct_path = \"/Users/katyscott/Documents/READII-2-ROQC/readii-fmcib/archive/temp_complete_output/procdata/RADCURE/images/mit_outputs/0_RADCURE-0020/CT/CT.nii.gz\"\n", | |
| "mask_path = \"/Users/katyscott/Documents/READII-2-ROQC/readii-fmcib/archive/temp_complete_output/procdata/RADCURE/images/mit_outputs/0_RADCURE-0020/RTSTRUCT_CT/GTV.nii.gz\"\n", | |
| from pathlib import Path | |
| ct_path = Path("data/test/CT.nii.gz") | |
| mask_path = Path("data/test/GTV.nii.gz") |
| # Current bounding box dimensions | ||
| current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z) | ||
| # bounding_box[1] - bounding_box[0], bounding_box[] |
There was a problem hiding this comment.
Fix undefined variables in bounding box dimensions calculation.
The variables max_x, min_x, etc. are used before they're extracted from the bounding box.
- # Current bounding box dimensions
- current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z)
- # bounding_box[1] - bounding_box[0], bounding_box[]
+ # Extract coordinates first
+ min_x, max_x, min_y, max_y, min_z, max_z = bounding_box
+ # Calculate current bounding box dimensions
+ current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Current bounding box dimensions | |
| current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z) | |
| # bounding_box[1] - bounding_box[0], bounding_box[] | |
| # Extract coordinates first | |
| min_x, max_x, min_y, max_y, min_z, max_z = bounding_box | |
| # Calculate current bounding box dimensions | |
| current_image_dimensions = (max_x - min_x, max_y - min_y, max_z - min_z) |
🧰 Tools
🪛 Ruff (0.8.2)
270-270: Undefined name max_x
(F821)
270-270: Undefined name min_x
(F821)
270-270: Undefined name max_y
(F821)
270-270: Undefined name min_y
(F821)
270-270: Undefined name max_z
(F821)
270-270: Undefined name min_z
(F821)
Made a new section under
processingfor images to addcrop.pyto.Includes cropping to bounding box, centroid, max dimension cube, and the existing PyRadiomics cropping method.
Will eventually be updated with better class structure, but this works for now.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores