[SyneRBI/SIRF] Quick fix for dicom output failure #1128 (PR #1141)
Evgueni Ovtchinnikov
Changes in this pull request
Enabled dicom output for MR reconstruction with Gadgetron.
Testing performed
Tested on examples.
Related issues
Quick fixes #1128.
Checklist before requesting a review
- I have performed a self-review of my code
- I have added docstrings/doxygen in line with the guidance in the developer guide
- I have implemented unit tests that cover any new or modified functionality
- The code builds and runs on my machine
- CHANGES.md has been updated with any functionality change
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following:
- The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.
You can view, comment on, or merge this pull request online at:
https://github.com/SyneRBI/SIRF/pull/1141
Commit Summary
- 87540f7 introduced a quick fix for dicom output failure #1128
File Changes
(7 files)
- M src/xGadgetron/cGadgetron/cgadgetron.cpp (8)
- M src/xGadgetron/cGadgetron/gadgetron_x.cpp (21)
- M src/xGadgetron/cGadgetron/include/sirf/Gadgetron/cgadgetron.h (2)
- M src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadget_lib.h (24)
- M src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_client.h (4)
- M src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h (7)
- M src/xGadgetron/pGadgetron/Gadgetron.py (13)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
best to add a test case for it (if easy)
In src/xGadgetron/cGadgetron/gadgetron_x.cpp:
> @@ -288,6 +296,7 @@ ImagesProcessor::process(const GadgetronImageData& images)
THROW("DICOM writer does not support complex images");
std::string config = xml();
+ std::cout << xml() << '\n';
i guess this has to be removed
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadget_lib.h:
> @@ -356,7 +356,29 @@ namespace sirf {
}
};
- class GenericReconCartesianFFTGadget : public Gadget {
+ class FFTGadget : public Gadget {
+ public:
+ FFTGadget() :
+ Gadget("FFT", "gadgetron_mricore", "FFTGadget")
+ {}
+ static const char* class_name()
+ {
+ return "FFTGadget";
+ }
+ };
+
+ class CombineGadget : public Gadget {
add some doxygen what this is supposed to do
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadget_lib.h:
> @@ -356,7 +356,29 @@ namespace sirf {
}
};
- class GenericReconCartesianFFTGadget : public Gadget {
+ class FFTGadget : public Gadget {
add some doxygen what this is supposed to do
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h:
> @@ -253,7 +253,7 @@ namespace sirf {
return "ImagesReconstructor";
}
- void process(MRAcquisitionData& acquisitions);
+ void process(MRAcquisitionData& acquisitions, const char* dcm_prefix = "image");
I'd prefer to have a set_dcm_prefix or similar function. Adding it as an optional argument is rather non-standard.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> @@ -1435,14 +1435,21 @@ def set_input(self, input_data):
'''
assert isinstance(input_data, AcquisitionData)
self.input_data = input_data
- def process(self):
+ def process(self, dcm_prefix=None):
I'd prefer to have a set_dcm_prefix or similar function. Adding it as an optional argument is rather non-standard.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> '''
Processes the input with the gadget chain.
+ dcm_prefix: Python text string.
+ If dcm_prefix is not None, the reconstructed images are written to
+ files <dcm_prefix>_<image number>.dcm.
+ Otherwise, they are stored in memory and can be retrieved by
Is it really "otherwise", i.e. will get_output() not work? that'd be a pity.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans pushed 1 commit.
- 6dc33e2 Merge branch 'master' into dicom-qfix
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
failures:
- https://github.com/SyneRBI/SIRF/actions/runs/3300780314/jobs/5445623930#step:8:999
cGT_reconstructImages() missing 1 required positional argument: 'dcm_prefix' - https://github.com/SyneRBI/SIRF/actions/runs/3300780314/jobs/5445623930#step:8:1080
Expected output of reconstruction to be complex
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 2 commits.
- 73dceb6 small corrections (missing/wrong dcm_prefix)
- 9b050c6 Merge branch 'dicom-qfix' of https://github.com/SyneRBI/SIRF into dicom-qfix
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 2 commits.
- e683917 git commit -m "optional dcm_prefix argument -> set_dcm_prefix() method"
- 7b6488d added dcm output option to fully sampled single chain demo
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h:
> @@ -253,7 +253,7 @@ namespace sirf {
return "ImagesReconstructor";
}
- void process(MRAcquisitionData& acquisitions);
+ void process(MRAcquisitionData& acquisitions, const char* dcm_prefix = "image");
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/gadgetron_x.cpp:
> @@ -288,6 +296,7 @@ ImagesProcessor::process(const GadgetronImageData& images)
THROW("DICOM writer does not support complex images");
std::string config = xml();
+ std::cout << xml() << '\n';
sure
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> '''
Processes the input with the gadget chain.
+ dcm_prefix: Python text string.
+ If dcm_prefix is not None, the reconstructed images are written to
+ files <dcm_prefix>_<image number>.dcm.
+ Otherwise, they are stored in memory and can be retrieved by
Yes, unfortunately the set of gadgets in currently used Gadgetron 3.17 does not seem to allow sending images to both the Gadgetron client and DICOM files. I might try to edit DicomFinishGadget so that it sends ISMRMRD images to the client but, assuming I succeed, do we want to ship our modification of Gadgetron with SIRF?
To remind you, this PR is just a quick fix, for a proper one we need help from Gadgetron people, but we can only get it once we moved to the recent Gadgetron.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
> @@ -1435,14 +1435,21 @@ def set_input(self, input_data):
'''
assert isinstance(input_data, AcquisitionData)
self.input_data = input_data
- def process(self):
+ def process(self, dcm_prefix=None):
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> '''
Processes the input with the gadget chain.
+ dcm_prefix: Python text string.
+ If dcm_prefix is not None, the reconstructed images are written to
+ files <dcm_prefix>_<image number>.dcm.
+ Otherwise, they are stored in memory and can be retrieved by
ok. as long as it's clear in the documentation. Maybe let get_output() check if the prefix was set, and if so raise a runtime error that's somewhat informative.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 2 commits.
- 381e946 small dicom-related amendments in MR ImageReconstructor
- f9b7a44 added brief descriptions of xml definition generators in gadget_lib.h [ci skip]
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadget_lib.h:
> @@ -356,7 +356,29 @@ namespace sirf {
}
};
- class GenericReconCartesianFFTGadget : public Gadget {
+ class FFTGadget : public Gadget {
+ public:
+ FFTGadget() :
+ Gadget("FFT", "gadgetron_mricore", "FFTGadget")
+ {}
+ static const char* class_name()
+ {
+ return "FFTGadget";
+ }
+ };
+
+ class CombineGadget : public Gadget {
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadget_lib.h:
> @@ -356,7 +356,29 @@ namespace sirf {
}
};
- class GenericReconCartesianFFTGadget : public Gadget {
+ class FFTGadget : public Gadget {
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> '''
Processes the input with the gadget chain.
+ dcm_prefix: Python text string.
+ If dcm_prefix is not None, the reconstructed images are written to
+ files <dcm_prefix>_<image number>.dcm.
+ Otherwise, they are stored in memory and can be retrieved by
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h:
> @@ -253,15 +253,31 @@ namespace sirf {
return "ImagesReconstructor";
}
+ void set_dcm_prefix(const char* dcm_prefix)
+ {
+ dcm_prefix_ = dcm_prefix;
+ }
+ std::string dcm_prefix() const
+ {
+ return dcm_prefix_;
+ }
+ bool dcm_output() const
+ {
+ return dcm_prefix_.size() > 1;
probably should be >0 I guess, but better to check !dcm_prefix.empty()
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h:
> @@ -253,15 +253,31 @@ namespace sirf {
return "ImagesReconstructor";
}
+ void set_dcm_prefix(const char* dcm_prefix)
+ {
+ dcm_prefix_ = dcm_prefix;
- add a check
dcm_prefixis not null. Although why do you want aconst char*. I'd vastly prefer aconst std::string&(it'll auto-convert anyway) - add a doxygen string that it needs to be set to an empty string to disable DICOM output.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> def process(self):
'''
Processes the input with the gadget chain.
+ dcm_prefix: Python text string.
+ If dcm_prefix is not None, the reconstructed images are written to
actually at the moment you set it "", not to None. are both handled the same?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 04bafa4 small dicom-related amendments/corrections
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h:
> @@ -253,15 +253,31 @@ namespace sirf {
return "ImagesReconstructor";
}
+ void set_dcm_prefix(const char* dcm_prefix)
+ {
+ dcm_prefix_ = dcm_prefix;
+ }
+ std::string dcm_prefix() const
+ {
+ return dcm_prefix_;
+ }
+ bool dcm_output() const
+ {
+ return dcm_prefix_.size() > 1;
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_x.h:
> @@ -253,15 +253,31 @@ namespace sirf {
return "ImagesReconstructor";
}
+ void set_dcm_prefix(const char* dcm_prefix)
+ {
+ dcm_prefix_ = dcm_prefix;
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/pGadgetron/Gadgetron.py:
> def process(self):
'''
Processes the input with the gadget chain.
+ dcm_prefix: Python text string.
+ If dcm_prefix is not None, the reconstructed images are written to
just forgot to change None (used in my first design) to "", corrected now
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans approved this pull request.
great. Feel free to merge whenever you're ready (after adding something to CHANGES.md)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- bec5456 [ci skip] updated CHANGES.md
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
Merged #1141 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.