feat: Add BaseWriter, and example NIFTIWriter with notebooks documenting them #84
Conversation
|
Warning Rate limit exceeded@jjjermiah has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications across multiple files, focusing on the configuration of linting rules in Changes
Possibly related PRs
Suggested reviewers
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 #84 +/- ##
==========================================
+ Coverage 44.51% 50.89% +6.37%
==========================================
Files 16 21 +5
Lines 739 894 +155
==========================================
+ Hits 329 455 +126
- Misses 410 439 +29 ☔ 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: 4
🧹 Outside diff range and nitpick comments (2)
src/readii/io/writers/nifti_writer.py (1)
70-70: Rename parameterPatientIDtopatient_idto follow PEP 8 naming conventions.According to PEP 8 guidelines, parameter names should be in lowercase with words separated by underscores. Changing
PatientIDtopatient_idimproves readability and maintains consistency across the codebase.src/readii/io/writers/README.md (1)
37-37: Correct the punctuation and formatting in the bullet point.There's an issue with the punctuation in the bullet point for
save(*args, **kwargs). It seems there's an extra period or incorrect indentation. Ensure that each bullet point is properly formatted:- - `resolve_path(**kwargs)`: Generates the file path by replacing placeholders in the - `filename_format`. - `save(*args, **kwargs)`: Abstract method. Subclasses implement the logic for writing files. + - `resolve_path(**kwargs)`: Generates the file path by replacing placeholders in the `filename_format`. + - `save(*args, **kwargs)`: Abstract method. Subclasses implement the logic for writing files.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: Loose punctuation mark.
Context: ...me_format. -save(*args, **kwargs)`: Abstract method. Subclasses implement t...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
notebooks/nifti_writer_example.pdfis excluded by!**/*.pdfnotebooks/writer_examples.pdfis excluded by!**/*.pdfpixi.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
config/ruff.toml(2 hunks)notebooks/nifti_writer_example.ipynb(1 hunks)notebooks/writer_examples.ipynb(1 hunks)src/readii/io/__init__.py(1 hunks)src/readii/io/writers/README.md(1 hunks)src/readii/io/writers/__init__.py(1 hunks)src/readii/io/writers/base_writer.py(1 hunks)src/readii/io/writers/nifti_writer.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/readii/io/writers/init.py
- src/readii/io/init.py
👮 Files not reviewed due to content moderation or server errors (2)
- notebooks/nifti_writer_example.ipynb
- notebooks/writer_examples.ipynb
🧰 Additional context used
📓 Learnings (1)
config/ruff.toml (1)
Learnt from: jjjermiah
PR: bhklab/readii#42
File: config/ruff.toml:19-21
Timestamp: 2024-11-12T13:18:03.441Z
Learning: In the 'readii' Python project, the team is incrementally adding files to the Ruff linter configuration in 'config/ruff.toml' to make small progress, rather than including all source files at once.
🪛 LanguageTool
src/readii/io/writers/README.md
[uncategorized] ~37-~37: Loose punctuation mark.
Context: ...me_format. - save(*args, **kwargs)`: Abstract method. Subclasses implement t...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (1)
config/ruff.toml (1)
8-8: Changes align with incremental updates to the linter configuration.
Including "src/readii/io/writers/**.py" in include and adding "notebooks/*" to extend-exclude is consistent with the team's approach of gradually adding files to the Ruff linter configuration. This allows for manageable progress while maintaining code quality.
Also applies to: 19-21
…emoval in BaseWriter
src/readii/io/writers/base_writer.py
Outdated
|
|
||
| filename_format: str = field(init=True) | ||
|
|
||
| DEFAULT_PATTERN: ClassVar[re.Pattern] = re.compile(r"%(\w+)|\{(\w+)\}") |
There was a problem hiding this comment.
Can you add a comment breaking down what this regex is at a high level?
There was a problem hiding this comment.
yup, moved to a utils directory and added more documentation throughout the module
Let me know if this is not confusing:
…d example would be I know this is just a dummy example, but wanted to update based on Sejin's feedback at the R2R project update meeting. Images now save in a modality specific directory and there's only one metadata file per patient since that's a "data_set" in this example.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
src/readii/io/utils/pattern_resolver.py (1)
97-101: Update theRaisessection in the docstring to reflect the correct exceptionIn the docstring of the
resolvemethod, theRaisessection mentionsValueError, but the code raisesPatternResolverError. For clarity and accuracy, the docstring should be updated to specifyPatternResolverErrorto match the actual exception being raised.tests/io/test_pattern_resolver.py (1)
2-2: Avoid unnecessary# type: ignorecommentsThe
# type: ignorecomment suppresses type checking for this import statement. It's better to resolve the underlying type issues to benefit from static type checking. Ensure that the imported modules have proper type annotations or adjust the type hints accordingly.tests/io/test_base_writer.py (2)
2-2: Remove unused importosThe
osmodule is imported but not used in the code. Removing unused imports improves code readability and reduces clutter.🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
79-81: Simplifywithstatements and remove unused variableYou can combine the nested
withstatements into a singlewithstatement for better readability. Additionally, the variablefile_pathis assigned but never used, since the exception is expected to be raised before it's utilized. Consider removing the assignment tofile_path.Apply this diff to simplify the code:
-def test_no_create_dirs_non_existent(temp_dir): - with pytest.raises(FileNotFoundError): - with SimpleWriter(root_directory=temp_dir / "nested_non_existent", filename_format="{date_time}.txt", create_dirs=False) as writer: - file_path = writer.save("Content") +def test_no_create_dirs_non_existent(temp_dir): + with pytest.raises(FileNotFoundError), \ + SimpleWriter(root_directory=temp_dir / "nested_non_existent", filename_format="{date_time}.txt", create_dirs=False): + pass🧰 Tools
🪛 Ruff (0.8.2)
79-80: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
81-81: Local variable
file_pathis assigned to but never usedRemove assignment to unused variable
file_path(F841)
tests/io/test_nifti_writer.py (3)
23-44: Add boundary test for minimum compression level.While the tests cover invalid cases well, consider adding a test for the minimum compression level (0) to ensure complete boundary testing.
def test_min_compression_level(): """Test for minimum compression level.""" writer = NIFTIWriter( root_directory=Path("TRASH"), filename_format="{PatientID}.nii.gz", compression_level=0, # Minimum compression level overwrite=False, create_dirs=True, ) assert writer.compression_level == 0
45-61: Add test for metadata validation in saved files.Consider adding a test to verify that the saved NIFTI file contains the expected metadata.
def test_save_with_metadata_validation(nifti_writer, sample_image): """Test that saved file contains expected metadata.""" patient_id = "12345" out_path = nifti_writer.save(sample_image, PatientID=patient_id) saved_image = sitk.ReadImage(str(out_path)) # Verify metadata preservation assert saved_image.GetSize() == sample_image.GetSize() assert saved_image.GetPixelID() == sample_image.GetPixelID()
62-80: Enhance parameterized tests with file characteristic verification.The tests verify file existence but could be enhanced to check compression and format-specific characteristics.
@pytest.mark.parametrize("compression_level", [0, 5, 9]) def test_save_with_different_compression_levels(nifti_writer, sample_image, compression_level): """Test saving with different compression levels.""" nifti_writer.compression_level = compression_level out_path = nifti_writer.save(sample_image, PatientID="12345") assert out_path.exists() # Verify file size characteristics if compression_level == 0: assert out_path.stat().st_size > 0 # Uncompressed should be larger saved_image = sitk.ReadImage(str(out_path)) assert saved_image.GetSize() == sample_image.GetSize()src/readii/io/writers/nifti_writer.py (2)
11-26: Enhance exception classes with error message templates.Consider adding error message templates to make error reporting more consistent.
class NiftiWriterValidationError(NiftiWriterError): """Raised when validation of writer configuration fails.""" - pass + def __init__(self, message: str, *args: object) -> None: + super().__init__(f"Validation Error: {message}", *args) class NiftiWriterIOError(NiftiWriterError): """Raised when I/O operations fail.""" - pass + def __init__(self, message: str, *args: object) -> None: + super().__init__(f"I/O Error: {message}", *args)
117-128: Move example code to documentation.The example code in the main block would be better placed in the module's documentation or a dedicated examples directory.
Consider moving this to a dedicated example file or adding it to the docstring as a usage example.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/readii/io/utils/__init__.py(1 hunks)src/readii/io/utils/pattern_resolver.py(1 hunks)src/readii/io/writers/base_writer.py(1 hunks)src/readii/io/writers/nifti_writer.py(1 hunks)tests/io/test_base_writer.py(1 hunks)tests/io/test_nifti_writer.py(1 hunks)tests/io/test_pattern_resolver.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/io/test_base_writer.py
2-2: os imported but unused
Remove unused import: os
(F401)
79-80: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
81-81: Local variable file_path is assigned to but never used
Remove assignment to unused variable file_path
(F841)
🔇 Additional comments (5)
src/readii/io/utils/__init__.py (1)
1-8: Module initialization is well-structured
The __init__.py file correctly initializes the module and specifies the public interface. The imports and __all__ variable are appropriately defined.
tests/io/test_nifti_writer.py (1)
6-21: LGTM! Well-structured test fixtures.
The fixtures provide a good foundation for testing, with appropriate sample data and default parameters.
src/readii/io/writers/base_writer.py (2)
100-101: Missing deletion of empty root directory in context manager exit.
The code logs that it's deleting the empty root directory but does not perform the deletion.
24-24:
Fix pattern_resolver field declaration.
The pattern_resolver should be an instance variable, not a class variable, as it's initialized per instance in post_init.
- pattern_resolver: PatternResolver = field(init=False)
+ pattern_resolver: PatternResolver = field(init=False, repr=False)Likely invalid or redundant comment.
src/readii/io/writers/nifti_writer.py (1)
106-108: Verify SimpleITK compression level support.
The compressionLevel parameter might not be supported by sitk.WriteImage.
✅ Verification successful
Let me search for more specific information about SimpleITK's WriteImage function and its compression support.
Let me check the SimpleITK imports and any related test files to understand the context better.
Let me check the test files, particularly test_nifti_writer.py, to see how the compression is being tested.
Based on the evidence gathered, I can now provide the final response:
The compressionLevel parameter is correctly implemented and tested
The codebase shows proper implementation and testing of SimpleITK's compression level support:
- The parameter is validated within bounds (0-9)
- Comprehensive tests verify writing with different compression levels (0, 5, 9)
- Tests pass successfully when writing compressed NIfTI files
- The implementation follows SimpleITK's documented API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SimpleITK compression level documentation
rg -A 5 "compression.*level"
Length of output: 4128
Script:
#!/bin/bash
# Search for SimpleITK WriteImage usage and documentation
rg -A 3 "sitk\.WriteImage"
# Search for any SimpleITK compression related tests or documentation
rg -i "simpleitk.*compress"
Length of output: 599
Script:
#!/bin/bash
# Search for SimpleITK imports and related test files
rg -A 5 "import.*sitk"
# Look for test files containing sitk
fd "test.*\.py" --exec rg -l "sitk"
# Check if there are any test files specifically for image writing
fd "test.*\.py" --exec rg -l "WriteImage"
Length of output: 5181
Script:
#!/bin/bash
# Check the test file content for compression testing
cat tests/io/test_nifti_writer.py
# Look for any SimpleITK documentation files
fd -e md -e rst -e txt --exec rg -l "SimpleITK"
Length of output: 3384
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/readii/io/utils/pattern_resolver.py (2)
11-15: Enhance the exception docstring.The docstring could be more informative by including:
- When this exception is raised
- Example usage
- List of possible error messages
class PatternResolverError(Exception): - """Base exception for errors in pattern resolution.""" + """Exception raised for errors in pattern resolution. + + This exception is raised when: + - The filename format pattern is invalid + - Required keys are missing during pattern resolution + - Context values contain None + + Examples + -------- + >>> resolver = PatternResolver("{invalid") + Raises: PatternResolverError("Invalid filename format: Unmatched brackets") + """
52-70: Consider improving attribute declarations and logging.The initialization logic is good, but could be enhanced for better maintainability and debugging.
@dataclass class PatternResolver: filename_format: str = field(init=True) + pattern_parser: PatternParser = field(init=False) + formatted_pattern: str = field(init=False) + keys: list[str] = field(init=False) DEFAULT_PATTERN: ClassVar[re.Pattern] = re.compile(r"%(\w+)|\{(\w+)\}") def __init__(self, filename_format: str) -> None: self.filename_format = filename_format try: self.pattern_parser = PatternParser( self.filename_format, pattern_parser=self.DEFAULT_PATTERN ) self.formatted_pattern, self.keys = self.parse() except InvalidPatternError as e: msg = f"Invalid filename format: {e}" raise PatternResolverError(msg) from e else: - logger.debug("All keys are valid.", keys=self.keys) - logger.debug("Formatted Pattern valid.", formatted_pattern=self.formatted_pattern) + logger.debug( + "Pattern validation successful", + filename_format=self.filename_format, + keys=self.keys, + formatted_pattern=self.formatted_pattern + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/readii/io/utils/pattern_resolver.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/readii/io/utils/pattern_resolver.py (1)
Learnt from: jjjermiah
PR: bhklab/readii#84
File: src/readii/io/utils/pattern_resolver.py:5-6
Timestamp: 2024-12-13T16:20:45.016Z
Learning: Dependencies on `med-imagetools` in `readii` are acceptable, as the package is intended to work off of `med-imagetools`.
🔇 Additional comments (4)
src/readii/io/utils/pattern_resolver.py (4)
1-9: LGTM! Clean import organization.
Imports are well-organized, with standard library imports separated from external dependencies.
17-50: Excellent documentation!
The class docstring is comprehensive, with clear examples and explanations of the pattern matching behavior.
71-87: LGTM! Well-documented method.
The parse method is clean, well-documented, and serves its purpose effectively.
1-120: Verify pattern resolution functionality across the codebase.
Let's ensure the pattern resolution is used consistently and correctly across the codebase.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get a complete picture.
Let me analyze the test file to ensure proper test coverage of the pattern resolution functionality.
Based on the comprehensive analysis of the code and test files, I can now generate the final response:
Pattern resolution functionality is well-implemented and properly tested
The pattern resolution implementation is robust and correctly used across the codebase:
- Comprehensive test suite covers various scenarios including error cases
- Properly integrated in BaseWriter as a core component for path resolution
- No instances of unsafe pattern formatting found that should be using PatternResolver
- Clear documentation and consistent usage pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of PatternResolver across the codebase
# and verify pattern formats being used.
# Find all pattern formats being used
echo "Checking pattern formats in use:"
rg -U "PatternResolver\(['\"].*?['\"]\)" --multiline
# Check for potential string formatting that might need to use PatternResolver
echo -e "\nChecking for potential missed opportunities to use PatternResolver:"
rg "f['\"].*?\{.*?\}['\"]" --multiline
# Look for any direct string formatting that might need validation
echo -e "\nChecking for direct string formatting that might need validation:"
rg "%.+?[sd]" --type py
Length of output: 5355
Script:
#!/bin/bash
# Check for actual instantiations and usage of PatternResolver
echo "Checking for PatternResolver instantiations and usage:"
rg -A 5 "PatternResolver\(" --type py
# Check for imports of PatternResolver
echo -e "\nChecking for imports of PatternResolver:"
rg "from.*PatternResolver import|import.*PatternResolver" --type py
# Look for any test files related to PatternResolver
echo -e "\nChecking for test files:"
fd "test.*pattern.*resolver" --type f
Length of output: 1606
Script:
#!/bin/bash
# Check the content of the test file
echo "Checking test file content:"
cat tests/io/test_pattern_resolver.py
# Check BaseWriter implementation to see how PatternResolver is used
echo -e "\nChecking BaseWriter implementation:"
cat src/readii/io/writers/base_writer.py
Length of output: 5427
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/io/test_nifti_writer.py (1)
4-4: Remove unused import 'Path'The imported
Pathfrompathlibis not used in this file. Removing it will clean up the code.Apply this diff to remove the unused import:
-from pathlib import Path🧰 Tools
🪛 Ruff (0.8.2)
4-4:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
src/readii/io/writers/nifti_writer.py (1)
115-117: Catch specific exceptions instead of 'Exception'Catching the base
Exceptioncan obscure the source of errors and make debugging difficult. Consider catching specific exceptions thatsitk.WriteImageorImageFileWritermight raise, such asRuntimeError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/readii/io/utils/pattern_resolver.py(1 hunks)src/readii/io/writers/nifti_writer.py(1 hunks)tests/io/test_nifti_writer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/readii/io/utils/pattern_resolver.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/io/test_nifti_writer.py
4-4: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
🔇 Additional comments (2)
src/readii/io/writers/nifti_writer.py (2)
91-98: Support for numpy array input implemented correctly
The addition of support for NumPy arrays in the save method enhances the versatility of the NIFTIWriter. This allows users to save images without explicitly converting arrays to SimpleITK images beforehand.
112-114:
Invalid use of compressionLevel parameter in sitk.WriteImage
The sitk.WriteImage function does not accept a compressionLevel parameter. Including it will result in a TypeError. To set the compression level, you should use sitk.ImageFileWriter instead.
Apply this change to use ImageFileWriter with compression level:
writer = sitk.ImageFileWriter()
writer.SetFileName(str(out_path))
writer.UseCompressionOn()
writer.SetCompressionLevel(self.compression_level)
writer.Execute(image)Replace lines 112-114 with the code above.
Mostly inspired and reusing logic from Med-ImageTools
DICOMSorterdesignBaseWriterstuff into MIT later so it can be reused around, but worth going through a review processSummary by CodeRabbit
New Features
BaseWriterfor writing text and CSV files.NIFTIWriterclass for managing NIFTI file writing with validation and error handling.CSVWriterclass for saving data in CSV format.Documentation
BaseWriterand its subclasses.Bug Fixes
Chores