SyneRBI / SIRF

Main repository for the CCP SynerBI software
http://www.ccpsynerbi.ac.uk
Other
57 stars 29 forks source link

MR SIRF-exercise fails with "non-empty container" after preprocessing #1237

Closed KrisThielemans closed 3 months ago

KrisThielemans commented 3 months ago

I tried acquisition_model_mr_pet_ct.ipynb (on a new VM, so maybe there's something wrong with that). It fails.

preprocessed_data = mr.preprocess_acquisition_data(mr_acq)

gives

ignoring acquisition 0
Message received with ID: 5
Input stream has terminated
Message received with ID: 5
Input stream has terminated
WARNING: cannot sort an empty container of acquisition data.

The CSM calculation then fails as the preprocessed data is empty.

@ckolbPTB any idea? Is it my VM?

KrisThielemans commented 3 months ago

Weirdly, the examples do work. I didn't check if the code is the same or not. For instance, this worked (i.e. no crashes and plots looked reasonable)

cd $SIRF_PATH/examples/Python/MR
python3 run_all.py
KrisThielemans commented 3 months ago

This is the same as #948, which we closed as it didn't occur anymore.

KrisThielemans commented 3 months ago

The examples use

mr_acq = mr.AcquisitionData(os.path.join(examples_data_path('MR'),'simulated_MR_2D_cartesian.h5

while the notebook uses

mr_acq = mr.AcquisitionData(os.path.join(examples_data_path('MR'),'grappa2_1rep.h5'))

Using the former makes the notebook work ok.

KrisThielemans commented 3 months ago

@DANAJK @ckolbPTB @evgueni-ovtchinnikov any ideas? Is there anything wrong with the example grappa data?

ckolbPTB commented 3 months ago

I guess the problem is that the labels of the acquisition does not fit to our current default flag settings.

Is grappa2_1rep.h5 something we simulate or have simulated in the past? Could it be outdated?

KrisThielemans commented 3 months ago

Checking git log --follow I get (ignoring some renames) https://github.com/SyneRBI/SIRF_data/commit/335987b1fe52c95a5cd5e2267542e5974c17b439 by @DANAJK

Date:   Sun May 6 18:33:32 2018 +0100

    Added two simulated data files

    Files created from MATLAB gen_us_data code. Files have 1 and 6 repetitions, GRAPPA undersampling and a relatively large amount of noise.

Relevant code is https://github.com/SyneRBI/tools/blob/master/gen_us_data.m.

DANAJK commented 3 months ago

The notebook I just saw uses 'ptb_resolutionphantom_GRAPPA4_ismrmrd.h5' ...?

I don't remember exactly what I did 6 years ago, but I probably ran the MATLAB function with two different settings and saved the output h5 files (which have since been renamed). This code also uses ISMRMRD MATLAB classes and would have been run with those available at the time.

KrisThielemans commented 3 months ago

https://github.com/SyneRBI/SIRF-Exercises/blob/master/notebooks/Introductory/acquisition_model_mr_pet_ct.ipynb uses the file I mentioned above. I cannot point to a line number for a notebook sadly.

Could be ISMRMRD version then of course.

If we're not too worried about this, then I think best course of action is:

  1. modify the notebook now
  2. release
  3. check if the problematic files are used anywhere at all. If not, delete them from SIRF_data for the next release.

Ok?

DANAJK commented 3 months ago

Yes, OK.

KrisThielemans commented 3 months ago

Here are the occurences in current SIRF

find SIRF/ -type f -exec grep --color -nH -e grappa2 {} +

SIRF/src/Synergistic/tests/CMakeLists.txt:49:   set(test_data "${CMAKE_SOURCE_DIR}/data/examples/MR/grappa2_1rep.h5" "${MR_vendor_dicom_as_nifti}" "${MR_SIRF_recon}")
SIRF/src/Synergistic/tests/CMakeLists.txt:51:   set(test_data "${CMAKE_SOURCE_DIR}/data/examples/MR/grappa2_1rep.h5" "${CMAKE_SOURCE_DIR}/data/examples/Registration/test2.nii.gz")
SIRF/examples/Python/MR/acquisition_data.py:15:                              (try e.g. --ignore=19 and --file=grappa2_6rep.h5)
SIRF/examples/Python/MR/acquisition_data.py:95:        # should see this if input data file is e.g. grappa2_6rep.h5
SIRF/examples/Python/PETMR/generate_MCIR_data.py:9:  -M <MR>, --MR=<MR>           template MR raw data (default: data/examples/MR/grappa2_1rep.h5)
SIRF/examples/Python/PETMR/generate_MCIR_data.py:58:    template_MR_raw_path = os.path.join(examples_data_path('MR'), 'grappa2_1rep.h5')
SIRF/data/README.md:5:`grappa2_1rep.h5`  created by running the MATLAB function `gen_us_data.m` in `tools` repository. SNR intenionally low so that noise is visible.
SIRF/data/README.md:6:`grappa2_6 rep.h5` as previous file but with 6 repetitions.
KrisThielemans commented 3 months ago

PR https://github.com/SyneRBI/SIRF-Exercises/pull/214 works fine.

However, using the MR data for gradient descent doesn't converge in the current implementation. Example with "relative step size" tau-.1 image (with the default tau=.3 it just oscillates).

Not sure if this is data related. @ckolbPTB @DANAJK do you know? If not, we probably need to create an issue on SIRF-Exercises. It's probably due to the choice of "relative" step-size.

DANAJK commented 3 months ago

I have tried to regenerate the h5 files using more modern ISMRMRD MATLAB code. The files can be found here

KrisThielemans commented 3 months ago

Using code in https://github.com/SyneRBI/SIRF-Exercises/pull/218, I no longer have oscillations when i set tau smaller. See there for plots with the new data.

I suggest we close this issue as there is no longer a problem with the preprocessing.