Skip to content
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

Upgrade from ISMRMRD to MRD v2 #2

Merged
merged 34 commits into from
Dec 14, 2024
Merged

Upgrade from ISMRMRD to MRD v2 #2

merged 34 commits into from
Dec 14, 2024

Conversation

naegelejd
Copy link
Member

No description provided.

@naegelejd
Copy link
Member Author

This is a very large PR that overhauls the entire Gadgetron code base to upgrade from ISMRMRD to MRDv2, switch to a pure streaming architecture, and remove socket networking from the codebase.

Specifically, this PR:

  1. Upgrades all tested Gadgets/Toolboxes to use MRDv2 instead of ISMRMRD
    • A few untested Gadgets remain because it would be better to add a test and keep them
  2. Removes all networking code
    • Removed gadgetron_ismrmrd_client
    • Greatly simplified apps/gadgetron
  3. Removes all Readers and Writers
    • gadgetron now reads/writes only the MRD Protocol (via stdin/stdout or --input/--output)
  4. Refactors end-to-end tests to use PyTest
    • Eliminated Siemens dat conversion
    • Test data has been converted to MRDv2 using test/integration/convert_test_data.py
  5. Removes everything Windows-related
  6. Ports the CI pipeline to GitHub Actions
    • Docker images are published only when a new release (tag) is pushed
    • Conda package is not yet published (it would conflict with existing gadgetron package)

@naegelejd naegelejd requested a review from hansenms November 26, 2024 14:57
hansenms
hansenms previously approved these changes Dec 11, 2024
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.

Overall this looks great. Huge amount of work here. Much more to do, but this sets us up with a basis to work off. Made some minor comments about some additional stuff we can remove and then maybe some things for next steps:

General Comments:

  • It would be good with a scheme to parallelize the data download for the tests.
  • Perhaps add a readme to say that the test case conversion tools are temporary. We will remove those once we have MRD data converters.
  • Next step should be removing XML files and runtime linking.
  • We should remove Doxygen (nobody needs that, intellisense is what we use) and Readthedocs and use approach from Tyger.
  • We should come up with some other approach to distributed.
  • We should have some general "tail" gadget where you choose output format with a parameter.

doc/source/conf.py Outdated Show resolved Hide resolved
doc/source/using.md Outdated Show resolved Hide resolved
gadgets/bart/BART_Recon.xml Outdated Show resolved Hide resolved
gadgets/moco/config/cpureg_cartesian_averaging.xml Outdated Show resolved Hide resolved
gadgets/python/legacy/config/pseudoreplica.xml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@naegelejd
Copy link
Member Author

The repository settings required a follow-up review. I addressed your feedback and opened issues for next steps.

@naegelejd naegelejd merged commit 936fded into main Dec 14, 2024
5 checks passed
@naegelejd naegelejd deleted the mrd2 branch December 14, 2024 15:10
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.

2 participants