Add ProtocolSerializer API for streaming#82
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new ProtocolSerializer API for streaming ISMRMRD objects, enabling serialization and deserialization of acquisitions, images, waveforms, headers, ndarrays, and text data to/from binary streams. The implementation includes comprehensive testing and example usage for streaming-based MRI data processing.
Key changes:
- New
ProtocolSerializerandProtocolDeserializerclasses for binary streaming - Comprehensive test suite covering all supported object types and interleaved serialization
- Stream copy utility and end-to-end reconstruction example
- Refactored common test utilities and removed deprecated warning behavior
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ismrmrd/serialization.py | Core implementation of protocol serialization classes |
| utilities/ismrmrd_copy_stream.py | Simple utility for copying ISMRMRD streams |
| tests/test_serialization.py | Comprehensive test suite for serialization functionality |
| examples/stream_recon.py | Full streaming reconstruction example with FFT processing |
| tests/end-to-end/validate_results.py | End-to-end test validation script |
| tests/end-to-end/test-reconstruction.sh | Bash script for comprehensive end-to-end testing |
| tests/test_common.py | Common test utilities moved from test_file.py |
| tests/test_file.py | Updated to use common utilities |
| ismrmrd/image.py | Converted deprecated warning to documentation comment |
| ismrmrd/init.py | Added serialization classes to public API |
| examples/demo.py | Removed old demo file |
| README.md | Updated with proper ISMRMRD link |
| .github/workflows/python-publish.yml | Added end-to-end test execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hansenms
left a comment
There was a problem hiding this comment.
Nice one. Thanks @naegelejd.
| "This function will be made consistent in a future version and this message " + | ||
| "will be removed." | ||
| ) | ||
| def matrix_size(self): |
There was a problem hiding this comment.
Are we no longer emitting this warning? I know we should stop threatening to remove this behavior, but what is the thinking about removing the warning? Just noisy?
There was a problem hiding this comment.
Yes, it's very noisy. I decided that this API is stable and solid and therefore nothing will change in a future version, contrary to the warning message. It's better to just document the API discrepancy.
No description provided.