Making read-all default for reading MR acquisitions from file.

evgueni-ovtchinnikov commented 9 months ago

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 AcquisitionProcessor chain (just to find out whether the chain needs AcquisitionFinishGadget) appears to fix the problem save for grappa2_1rep.h5 and grappa2_6rep.h5 data. Possible reason for failures with the latter data is the absence of the measurementInformation in xml headers. Assuming this is so, we can now use the proper read-all default when reading MR acquisition data, fixing grappa2 data to be addressed by a separate issue.

Testing performed

Tested on meas_MID80_2d_cart_cine_30ph_headcoil_FID17006.h5 and 3D_RPE_Lowres.h5 data provided by @ckolbPTB and @johannesmayer.

Related issues

Fails on grappa2_1rep.h5 and grappa2_6rep.h5 by @DANAJK . Possible reason is the absence of the measurementInformation in xml section - 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

[ ] 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
[x] 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:

[x] 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.

KrisThielemans commented 9 months ago

Did this PR branch of from the wrong branch? Probably needs to be rebased on master, or alternatively master merged on here.

KrisThielemans commented 8 months ago

@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.

ckolbPTB commented 8 months ago

@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?

evgueni-ovtchinnikov commented 8 months ago

@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 ignore-acq: actually, only 4 last commits are for ignore-acq-fix-noise!

KrisThielemans commented 8 months ago

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)

ckolbPTB commented 8 months ago

@evgueni-ovtchinnikov thank you and yes

KrisThielemans commented 8 months ago

great. @evgueni-ovtchinnikov I suggest you "squash and merge" then (click on the arrow on the green button)

SyneRBI / SIRF