Add HDF5 support to Python ISMRMRD -> MRD converter#54
Add HDF5 support to Python ISMRMRD -> MRD converter#54
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Python ismrmrd_to_mrd converter so it can convert directly from an ISMRMRD HDF5 file into an MRD stream, and updates end-to-end tests to cover ISMRMRD↔MRD roundtrips for both the C++ and Python converters.
Changes:
- Add
--datasetmode topython/mrd/tools/ismrmrd_to_mrd.pyto convert ISMRMRD HDF5 directly to MRD. - Extend
test/test-all.shto validate ISMRMRD↔MRD roundtrip equivalence (including recon stream cases). - Remove redundant converter-specific just targets and pin Miniforge version in the conda workflow.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
python/mrd/tools/ismrmrd_to_mrd.py |
Adds HDF5 dataset conversion path and updates CLI plumbing. |
test/test-all.sh |
Expands end-to-end coverage with ISMRMRD↔MRD conversion/roundtrip checks. |
justfile |
Removes standalone converter test recipes and relies on end-to-end coverage. |
.github/workflows/mrd_conda.yml |
Pins Miniforge version for more reproducible CI setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -2,6 +2,7 @@ | |||
| """Convert ISMRMRD stream to MRD stream.""" | |||
There was a problem hiding this comment.
The module docstring still says this tool converts an ISMRMRD stream only, but main() now also supports converting directly from an ISMRMRD HDF5 dataset (--dataset). Please update the docstring to reflect the expanded functionality so generated docs/help stay accurate.
| """Convert ISMRMRD stream to MRD stream.""" | |
| """Convert ISMRMRD data (stream or HDF5 dataset) to an MRD stream.""" |
| # Try integer first | ||
| try: | ||
| int_val = int(str_val) | ||
| # Check if it's actually an integer (not a float like "3.0") | ||
| if '.' not in str_val and 'e' not in str_val.lower(): | ||
| if str_val.isdigit(): | ||
| try: | ||
| int_val = int(str_val) | ||
| meta_values.append(mrd.ImageMetaValue.Int64(int_val)) |
There was a problem hiding this comment.
str_val.isdigit() only returns true for unsigned integer strings, so metadata values like "-1"/"+3" will no longer be emitted as Int64 and will instead fall through to the float or string paths. This is a behavioral regression from the previous int()-based parsing; consider using a sign-aware integer check (or the earlier int() attempt with an explicit float-notation guard) to preserve integer typing.
| ismrmrd_hdf5_to_stream -i phantom.h5 --use-stdout | ismrmrd_stream_recon_cartesian_2d --use-stdin --use-stdout | ismrmrd_to_mrd | mrd_to_ismrmrd > recon_rountrip.ismrmrd | ||
| diff direct.ismrmrd roundtrip.ismrmrd && diff recon_direct.ismrmrd recon_rountrip.ismrmrd |
There was a problem hiding this comment.
Typo in the output filename recon_rountrip.ismrmrd (missing 'd' in 'roundtrip'). Even though it works, it makes the test harder to read/search; consider renaming consistently to recon_roundtrip.ismrmrd.
| ismrmrd_hdf5_to_stream -i phantom.h5 --use-stdout | ismrmrd_stream_recon_cartesian_2d --use-stdin --use-stdout | python -m mrd.tools.ismrmrd_to_mrd | python -m mrd.tools.mrd_to_ismrmrd > recon_rountrip.ismrmrd | ||
| python diff-ismrmrd-streams.py recon_direct.ismrmrd recon_rountrip.ismrmrd |
There was a problem hiding this comment.
Typo in the output filename recon_rountrip.ismrmrd (missing 'd' in 'roundtrip'). Even though it works, it makes the test harder to read/search; consider renaming consistently to recon_roundtrip.ismrmrd.
1 participant
This enables converting directly from ISMRMRD HDF5 to an MRD stream, without having to to convert to the intermediate ISMRMRD stream format.
Example usage: