-
Notifications
You must be signed in to change notification settings - Fork 29
Add ProtocolSerializer API for streaming #82
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
Conversation
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.