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

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

Closed
KrisThielemans opened this issue Mar 10, 2024 · 13 comments
Closed
Labels
Milestone

Comments

@KrisThielemans
Copy link
Member

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 KrisThielemans added this to the v3.6 milestone Mar 10, 2024
@KrisThielemans
Copy link
Member Author

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
Copy link
Member Author

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

@KrisThielemans
Copy link
Member Author

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
Copy link
Member Author

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

@ckolbPTB
Copy link
Contributor

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
Copy link
Member Author

Checking git log --follow I get (ignoring some renames) SyneRBI/SIRF_data@335987b 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
Copy link
Contributor

DANAJK commented Mar 12, 2024

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
Copy link
Member Author

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
Copy link
Contributor

DANAJK commented Mar 12, 2024

Yes, OK.

@KrisThielemans
Copy link
Member Author

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 added a commit to KrisThielemans/SIRF-Exercises that referenced this issue Mar 15, 2024
The original data was created with an old ISMRMRD and it no
longer seems to work.

See SyneRBI/SIRF#1237
@KrisThielemans
Copy link
Member Author

PR SyneRBI/SIRF-Exercises#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.

KrisThielemans added a commit to SyneRBI/SIRF-Exercises that referenced this issue Mar 15, 2024
The original data was created with an old ISMRMRD and it no
longer seems to work.

See SyneRBI/SIRF#1237
@DANAJK
Copy link
Contributor

DANAJK commented Mar 16, 2024

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

@KrisThielemans
Copy link
Member Author

Using code in SyneRBI/SIRF-Exercises#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants