Multi-modality intro notebooks

[ckolbPTB]

I added MR and CT to the introductory/introduction notebook (should fix issue #61)

I also added a new notebook to show forward/backward for CT, MR and PET (should fix issue #65)

Would be great if somebody could go through the notebook and check that everything I wrote especially about CT and PET is actually true.

Both brainweb and PET create temporary files. Currently they are created in the notebook/introductory folder. Not sure what would be a better location for them.

[DANAJK]

I get an error when trying to import brainweb while running the notebook acquisition_model_mr_pet_ct. pip implied brainweb was already installed. Oddly I don't get that import error for the BrainWeb notebooks in the Synergisitic folder - though they fail in the next cell with a dead kernel.

The above is using a Jupyter notebook in my local browser with SIRF from a Docker container.

[ckolbPTB]

Did you manage to run the introduction notebook?

[DANAJK]

No.

# Make sure everything is installed that we need
!pip install brainweb nibabel --user

Requirement already satisfied: brainweb in /home/sirfuser/.local/lib/python3.8/site-packages (1.6.2)
Requirement already satisfied: nibabel in /opt/pyvenv/lib/python3.8/site-packages (3.2.1)
Requirement already satisfied: numpy .....
.....

# Initial imports etc
import numpy
from numpy.linalg import norm
import matplotlib.pyplot as plt
import matplotlib.animation as animation
import os
import sys
import shutil
import brainweb
from tqdm.auto import tqdm

from sirf.Utilities import examples_data_path

ModuleNotFoundErrorTraceback (most recent call last)
<ipython-input-16-fcf969975e47> in <module>
      7 import sys
      8 import shutil
----> 9 import brainweb
     10 from tqdm.auto import tqdm
     11 

ModuleNotFoundError: No module named 'brainweb'

[KrisThielemans]

more than likely it's calling the wrong pip. I guess the jupyter environment isn't called in the appropriate virtualenv?

you could try with python3 -m pip install... first to see if that's it. Better thing to do would be to create a requirements.txt and add it, and things like numba. @ashgillman can you help with that?

then we "only" need to make sure that the requirements.txt is used in both the VM and docker files. Please create an issue in both (i.e. "if it's present, use it").

[DANAJK]

That didn't work, but this before the import brainweb gets me a bit further

import sys
sys.path.append("/home/sirfuser/.local/lib/python3.8/site-packages")

Now I'm getting a dead kernel with no other error messages in the notebook or terminal in this bit:

for f in tqdm([fname], desc="mMR ground truths", unit="subject"):
    vol = brainweb.get_mmr_fromfile(f, petNoise=1, t1Noise=0.75, t2Noise=0.75, petSigma=1, t1Sigma=1, t2Sigma=1)

[ashgillman]

Sure, I can do this Friday. Would you want a requirements.txt within the subdirectory you mean that installs the dependencies for the exercises? Or a global one?

David - it seems strange you had to add that... Can you also quickly check print(sys.path) and print(sys.version) - I wonder if your pip is 3.8 and you are running a different version, as Kris suggests, which is another error/issue.

Dead kernel could imply Out-of-Memory killer has killed your notebook? You may need to allocate more to the VM.

Less likely, but it could be from sys.path.append("/home/sirfuser/.local/lib/python3.8/site-packages") if you aren't running Python 3.8 - numpy or scipy or something at the c-level might be crashing with a version conflict.

[paskino]

You used the methods forward and backward. These don’t exist in CIL as the AcquisitionModel does not exist in CIL and what exists is the LinearOperatorwith methods direct and adjoint.

In SIRF we’ve aliased direct and adjoint to forward and backward respectively. I wonder if you don't want to mention this in the notebook.

[KrisThielemans]

great stuff. some comments on the acq_model notebook

• Misspelling _MR/c_coilcombindation.ipynb
• im_mr gets created twice (also from csm)
• potentially drop set_num_tangential_LORs from this notebook
• no idea what the abbreviation bw_ig is supposed to stand for. (I personally prefer slightly longer variables names overall...)
• "Let's create an ImageData object for each modality and display them" I suggest saying "and display the middle slice of each"
• Do not call the backprojections "rec_pet", but maybe "backprojected_pet"

[paskino]

bw_ig stands for brainweb image geometry. In our notebooks we tend to name the ImageGeometry instances ig, so maybe it's better to use ig or ct_ig ~or ct_am_template~?

An ImageGeometry loosely corresponds to an ~AcquisitionModel~ ImageData template.

An AcquisitionGeometry loosely corresponds to an AcquisitionModel template.

[KrisThielemans]

@ashgillman you wrote

Would you want a requirements.txt within the subdirectory you mean that installs the dependencies for the exercises?

one for SIRF-Exercises. We have one for SIRF already. (not sure about CIL)

[KrisThielemans]

@ckolbPTB @edo we'll need to change the README and INSTALL to say that some exercises use CIL. They can choose not to use it, but will need to edit the relevant exercises then.

[KrisThielemans]

An ImageGeometry loosely corresponds to an AcquisitionModel template.

@edo that is a bit weird. ImageGeometry should correspond to ImageData, AcquisitionGeometry (or whatever) to AcquisitionData...

[DANAJK]

I think pip is putting brainweb into a place not on the path. From what I think is a Docker restart I get the below. The only other odd thing is that in my Docker dashboard I seem to be running "service" which was ancient on Docker Hub until today (and I never knowingly downloaded it). I suspect I accidentally built it locally but there is so much automagic stuff going on its hard to know.

import sys
print(sys.path)
print(sys.version) 

['/devel/SIRF-Exercises/notebooks/Introductory', '/opt/SIRF-SuperBuild/INSTALL/python', '/opt/pyvenv/lib/python38.zip', '/opt/pyvenv/lib/python3.8', '/opt/pyvenv/lib/python3.8/lib-dynload', '', '/opt/pyvenv/lib/python3.8/site-packages', '/opt/pyvenv/lib/python3.8/site-packages/IPython/extensions', '/home/sirfuser/.ipython']
3.8.8 | packaged by conda-forge | (default, Feb 20 2021, 16:22:27) 
[GCC 9.3.0]

# Make sure everything is installed that we need
!python3 -m pip install brainweb nibabel --user

Collecting brainweb
  Downloading brainweb-1.6.2-py2.py3-none-any.whl (11 kB)
Requirement already satisfied: nibabel in /opt/pyvenv/lib/python3.8/site-packages (3.2.1)
Collecting scikit-image
  Downloading scikit_image-0.18.1-cp38-cp38-manylinux1_x86_64.whl (30.2 MB)
     |████████████████████████████████| 30.2 MB 8.4 MB/s eta 0:00:01
Requirement already satisfied: requests in /opt/pyvenv/lib/python3.8/site-packages (from brainweb) (2.25.1)
Requirement already satisfied: numpy in /opt/pyvenv/lib/python3.8/site-packages (from brainweb) (1.20.2)
Requirement already satisfied: tqdm>=4.42.0 in /opt/pyvenv/lib/python3.8/site-packages (from brainweb) (4.60.0)
Requirement already satisfied: packaging>=14.3 in /opt/pyvenv/lib/python3.8/site-packages (from nibabel) (20.9)
Requirement already satisfied: pyparsing>=2.0.2 in /opt/pyvenv/lib/python3.8/site-packages (from packaging>=14.3->nibabel) (2.4.7)
Requirement already satisfied: certifi>=2017.4.17 in /opt/pyvenv/lib/python3.8/site-packages (from requests->brainweb) (2020.12.5)
Requirement already satisfied: chardet<5,>=3.0.2 in /opt/pyvenv/lib/python3.8/site-packages (from requests->brainweb) (4.0.0)
Requirement already satisfied: urllib3<1.27,>=1.21.1 in /opt/pyvenv/lib/python3.8/site-packages (from requests->brainweb) (1.26.4)
Requirement already satisfied: idna<3,>=2.5 in /opt/pyvenv/lib/python3.8/site-packages (from requests->brainweb) (2.10)
Requirement already satisfied: scipy>=1.0.1 in /opt/pyvenv/lib/python3.8/site-packages (from scikit-image->brainweb) (1.6.2)
Collecting imageio>=2.3.0
  Downloading imageio-2.9.0-py3-none-any.whl (3.3 MB)
     |████████████████████████████████| 3.3 MB 9.0 MB/s eta 0:00:01
Collecting PyWavelets>=1.1.1
  Downloading PyWavelets-1.1.1-cp38-cp38-manylinux1_x86_64.whl (4.4 MB)
     |████████████████████████████████| 4.4 MB 7.6 MB/s eta 0:00:01
Collecting tifffile>=2019.7.26
  Downloading tifffile-2021.4.8-py3-none-any.whl (165 kB)
     |████████████████████████████████| 165 kB 8.9 MB/s eta 0:00:01
Requirement already satisfied: matplotlib!=3.0.0,>=2.0.0 in /opt/pyvenv/lib/python3.8/site-packages (from scikit-image->brainweb) (3.4.1)
Collecting networkx>=2.0
  Downloading networkx-2.5.1-py3-none-any.whl (1.6 MB)
     |████████████████████████████████| 1.6 MB 9.8 MB/s eta 0:00:01
Requirement already satisfied: pillow!=7.1.0,!=7.1.1,>=4.3.0 in /opt/pyvenv/lib/python3.8/site-packages (from scikit-image->brainweb) (8.2.0)
Requirement already satisfied: python-dateutil>=2.7 in /opt/pyvenv/lib/python3.8/site-packages (from matplotlib!=3.0.0,>=2.0.0->scikit-image->brainweb) (2.8.1)
Requirement already satisfied: kiwisolver>=1.0.1 in /opt/pyvenv/lib/python3.8/site-packages (from matplotlib!=3.0.0,>=2.0.0->scikit-image->brainweb) (1.3.1)
Requirement already satisfied: cycler>=0.10 in /opt/pyvenv/lib/python3.8/site-packages (from matplotlib!=3.0.0,>=2.0.0->scikit-image->brainweb) (0.10.0)
Requirement already satisfied: six in /opt/pyvenv/lib/python3.8/site-packages (from cycler>=0.10->matplotlib!=3.0.0,>=2.0.0->scikit-image->brainweb) (1.15.0)
Collecting decorator<5,>=4.3
  Downloading decorator-4.4.2-py2.py3-none-any.whl (9.2 kB)
Installing collected packages: decorator, tifffile, PyWavelets, networkx, imageio, scikit-image, brainweb
  WARNING: The scripts lsm2bin, tiff2fsspec, tiffcomment and tifffile are installed in '/home/sirfuser/.local/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
  WARNING: The scripts imageio_download_bin and imageio_remove_bin are installed in '/home/sirfuser/.local/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
  WARNING: The script skivi is installed in '/home/sirfuser/.local/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
Successfully installed PyWavelets-1.1.1 brainweb-1.6.2 decorator-4.4.2 imageio-2.9.0 networkx-2.5.1 scikit-image-0.18.1 tifffile-2021.4.8

and re-running the !python3 -m pip install ...

Requirement already satisfied: brainweb in /home/sirfuser/.local/lib/python3.8/site-packages (1.6.2)
...

Then import brainweb gives

ModuleNotFoundErrorTraceback (most recent call last)
<ipython-input-7-80312b2b3d17> in <module>
----> 1 import brainweb

ModuleNotFoundError: No module named 'brainweb'

[KrisThielemans]

From what I think is a Docker restart

From our docker README I believe you can get rid of history of your container by using

docker rm sirf

AFAIK This will remove the container, but not the image ) i.e. the original files will remain such that you don't need another download from Docker Hub, but whatever you did with that container will be removed). See also https://github.com/SyneRBI/SIRF-SuperBuild/tree/master/docker#stopping-and-removal

If you're not sure which version you're running, we're in a bit of trouble. I'm not sure myself how to check (aside from the links above). It isn't helped by the fact that (quoting from the end of the README)

Currently all compose files call the container sirf. You could edit the .yml file if you want to run different versions.

My latest guess is that your actual problem is because the python kernel is running in a virtualenv (in /opt/pyvenv) and that the system call isn't, some I am not sure why that would be the case though as we activate the virtualenv in our bashrc and (virtual env doc implies that the pip install should be ok,(Warning: I don't really understand virtualenv well enough and in particular why we'd have one inside a docker environment).

In any case, I hope that by pre-installing brainweb this problem would disappear. I've created #77.

By the way, in your install log, I notice

WARNING: The scripts imageio_download_bin and imageio_remove_bin are installed in '/home/sirfuser/.local/bin' which is not on PATH. Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.

Presumably this will hit us at some point. I think we should change our docker bashrc.- I've created https://github.com/SyneRBI/SIRF-SuperBuild/issues/525

[paskino]

An ImageGeometry loosely corresponds to an AcquisitionModel template.

@edo that is a bit weird. ImageGeometry should correspond to ImageData, AcquisitionGeometry (or whatever) to AcquisitionData...

Of course! I corrected my comment.

[ckolbPTB]

In SIRF we’ve aliased direct and adjoint to forward and backward respectively. I wonder if you don't want to mention this in the notebook.

I added a paragraph on this in the notebook. @paskino could you please check that what I wrote is actually correct?

[ckolbPTB]

great stuff. some comments on the acq_model notebook

* Misspelling _MR/c_coilcombindation.ipynb

* im_mr gets created twice (also from csm)

* potentially drop set_num_tangential_LORs from this notebook

* no idea what the abbreviation bw_ig is supposed to stand for. (I personally prefer slightly longer variables names overall...)

* "Let's create an ImageData object for each modality and display them" I suggest saying "and display the middle slice of each"

* Do not call the backprojections "rec_pet", but maybe "backprojected_pet"

I implemented all the suggestions. I am not so sure about the last one. I agree that "backprojected_pet" makes more sense than "rec_pet" but then I guess I would have to call the mr "fourier_transformed_with_coil_summation_mr". @KrisThielemans Would it make sense to call them "im_estimate_pet/mr/ct" or just "bwd_pet/mr/ct"?

[ckolbPTB]

@ckolbPTB @edo we'll need to change the README and INSTALL to say that some exercises use CIL. They can choose not to use it, but will need to edit the relevant exercises then.

I adapted the README with the new notebook and mentioned all modalities. I am not sure where we would add the information about how to install CIL/refer to the page where there is information about how to install CIL.

We don't just need CIL but also CIL with the astra toolbox, which is not available in the standard VM. We discussed if we could change this, such that astra is always there. Any more thoughts on this?

[KrisThielemans]

We don't just need CIL but also CIL with the astra toolbox, which is not available in the standard VM. We discussed if we could change this, such that astra is always there. Any more thoughts on this?

I believe @paskino is working on this (see e.g. https://github.com/SyneRBI/SIRF-SuperBuild/pull/510)

[KrisThielemans]

I am not so sure about the last one. I agree that "backprojected_pet" makes more sense than "rec_pet" but then I guess I would have to call the mr "fourier_transformed_with_coil_summation_mr". @KrisThielemans Would it make sense to call them "im_estimate_pet/mr/ct" or just "bwd_pet/mr/ct"?

😄 I guess we have a problem here that many people will not know what an adjoint is (certainly at this stage of the notebooks). Explaining that seems to belong more in a "basic image reconstruction" notebook, which we could then refer to here.

As we call the operator backward, I agree with your name bwd_pet etc. (This seems to be a particular case where the longer name (backward_pet) doesn't look appropriate!)

[KrisThielemans]

I am not so sure about the last one. I agree that "backprojected_pet" makes more sense than "rec_pet" but then I guess I would have to call the mr "fourier_transformed_with_coil_summation_mr". @KrisThielemans Would it make sense to call them "im_estimate_pet/mr/ct" or just "bwd_pet/mr/ct"?

😄 I guess we have a problem here that many people will not know what an adjoint is (certainly at this stage of the notebooks).

[paskino]

We don't just need CIL but also CIL with the astra toolbox, which is not available in the standard VM. We discussed if we could change this, such that astra is always there. Any more thoughts on this?

I could add CIL + ASTRA to the VM. https://github.com/SyneRBI/SIRF-SuperBuild/pull/510 is about setting a later version of CIL in the SuperBuild, but I was blocked by a bug in a unittest. It should be correct now.

[paskino]

In SIRF we’ve aliased direct and adjoint to forward and backward respectively. I wonder if you don't want to mention this in the notebook.

I added a paragraph on this in the notebook. @paskino could you please check that what I wrote is actually correct?

@ckolbPTB I think it's all fine. Of course there aren't many details, but I think they'd be really beyond the objective of this notebook.

[ckolbPTB]

As we call the operator backward, I agree with your name bwd_pet etc. (This seems to be a particular case where the longer name (backward_pet) doesn't look appropriate!)

I changed everything to bwd_pet/ct/mr

[KrisThielemans]

ready to merge? (might need adjustments later of course)

[ckolbPTB]

I simplified the CT acquisition model a bit as suggested by @paskino. From my side everything should be ready to merge now.