Making read-all default for reading MR acquisitions from file. #1227
Conversation
remove `-e` from the ctest line preventing unit test to complete.
…Data Failures cased by noise calibration data forced ignoring it, which was a poor solution. It turned out that ignoring these data just on the preliminary test run in AcquisitionProcessor chain (to find out whether the chain needs AcquisitionFinishGadget) appears to fix the problem save for grappa2_1rep.h5 an grappa2_6rep.h5 data. Possible reason for failures with the latter data is the absence of the measurementInformation in xml headers. Assuming this is true, we can now use the proper read-all-acquisition default when reading MR acquisition data, fixing grappa2 data to be addressed by a separate issue.
|
Did this PR branch of from the wrong branch? Probably needs to be rebased on |
|
@ckolbPTB @evgueni-ovtchinnikov where are we with this one? It seems small enough. If it is ready, I suggest to squash-merge it, as it has a very large history but only few surviving changes. |
|
@evgueni-ovtchinnikov do you think you could run the MR exercises (https://github.com/SyneRBI/SIRF-Exercises/tree/master/notebooks/MR) to make sure we can still reconstruct the data with this change? |
|
@ckolbPTB all MR notebooks run ok - permission to merge? @KrisThielemans the history is large because GitHub somehow figured out that this branch is a continuation of |
|
We could sort out the history, or just squash (i.e. all changes will be entered as a single commit). It's not a great idea if others have incorporated this branch in their work, but I guess that's unlikely in this case (and they could speak up now). If you do squash merge, please edit the suggested commit message such that it makes sense (as github will just list all the commit messages anyway) |
|
@evgueni-ovtchinnikov thank you and yes |
|
great. @evgueni-ovtchinnikov I suggest you "squash and merge" then (click on the arrow on the green button) |
Changes in this pull request
Failures cased by noise calibration data forced ignoring it, which was a poor solution. It turns out that ignoring these data just on the preliminary test run in
AcquisitionProcessorchain (just to find out whether the chain needsAcquisitionFinishGadget) appears to fix the problem save forgrappa2_1rep.h5andgrappa2_6rep.h5data. Possible reason for failures with the latter data is the absence of themeasurementInformationinxmlheaders. Assuming this is so, we can now use the proper read-all default when reading MR acquisition data, fixinggrappa2data to be addressed by a separate issue.Testing performed
Tested on
meas_MID80_2d_cart_cine_30ph_headcoil_FID17006.h5and3D_RPE_Lowres.h5data provided by @ckolbPTB and @johannesmayer.Related issues
Fails on
grappa2_1rep.h5andgrappa2_6rep.h5by @DANAJK . Possible reason is the absence of themeasurementInformationinxmlsection - an issue to be created.For some reason, GitHub decided to treat this PR as a continuation of already merged #1207 - all the commits listed below up to 8343cd3 are actually from #1207.
Checklist before requesting a review
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following: