Skip to content

Conversation

@naegelejd
Copy link
Contributor

No description provided.

@naegelejd naegelejd requested a review from Copilot September 9, 2025 16:37
Copy link

Copilot AI left a 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 ProtocolSerializer and ProtocolDeserializer classes 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.

Copy link
Member

@hansenms hansenms left a 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):
Copy link
Member

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?

Copy link
Contributor Author

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.

@naegelejd naegelejd merged commit d6adb42 into master Sep 12, 2025
4 checks passed
@naegelejd naegelejd deleted the naegelejd/protocol-serializer branch September 12, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants