Suggestions for MAPEM and reconstruct_measured_data #140

ashgillman · 2021-06-28T05:08:58Z

No description provided.

ashgillman · 2021-06-28T05:11:48Z

notebooks/PET/MAPEM.ipynb

@@ -254,7 +254,7 @@
    "# Implement MAP-EM!\n",
    "Actually, we will implement MAP-OSEM as that's easy to do.\n",
    "\n",
-    "The lines below (almost) implement MAP-OSEM with a prior which just smooths along the horizontal direction. Either evaluate, and/or extend to 2D, and/or increase the speed of your Python code.\n",
+    "The lines below (almost) implement MAP-OSEM with a prior which just smooths along the horizontal direction.\n",


I'm not convinced that the current compute_xreg would actually be made much faster by tweaking the implementation - and the use of jit would make it unpredictable. I don't know if its a good idea to suggest that the participants try to optimise it.

Extending to 2D is a good suggestion, but its already below in the "what now?" section

notebooks/PET/MAPEM.ipynb

ashgillman · 2021-06-28T05:13:34Z

notebooks/PET/MAPEM.ipynb

@@ -292,7 +294,7 @@
   "cell_type": "markdown",
   "metadata": {},
   "source": [
-    "We will use the \"Just In Time\" compilation feature if `numba` here to speed-up the following function. (There are faster ways to write it, but this might be easier to understand)."
+    "We will use the \"Just In Time\" (jit) compilation feature of `numba` here to speed-up the following function."


Again, I'm not sure that this actually is that slow. I think jit would have every reason to be able to optimise this well.

ashgillman · 2021-06-28T05:15:27Z

notebooks/PET/reconstruct_measured_data.ipynb

+    "# This command prepends the data path to the data file so that the header in our working folder points to the data\n",
+    "# You likely won't need to do this for your own data if the data file is in the same directory.\n",
+    "!sed -i.bak2 -e \"s#\\(!name of data file:=\\)#\\\\1${data_path}/#\" umap.v.hdr\n",
+    "!sed -i.bak2 -e \"s#\\(!name of data file:=\\)#\\\\1${data_path}/#\" norm.v.hdr\n",


All this seding is probably already confusing to the reader.

I've made the two sed lines more similar to one-another rather that confusingly using jupyter commands on the first one and os.system calls on the second

I'm assuming you checked it still worked ok?

Of course, they can be combined in1, but that probably doesn't help the user.

could add a note that this is needed due to limitations of the current convert* scripts in STIR if you think that'd help

Yes, tested.

this is needed due to limitations of the current convert* scripts

Well normally you'd run this and still have the file in the same directory - I don't think we'd want the convert* scripts to convert to absolute files (makes a pain for network directories, moving data, etc.) anyway. So I think the current comment communicates that

ashgillman · 2021-06-28T05:15:54Z

notebooks/PET/reconstruct_measured_data.ipynb

+    "# Advanced: if you'd like to have a look at what changed in the umap, uncomment below\n",
+    "# Lines starting with < are the original Siemens\n",
+    "# and lines starting with > are the STIR converted\n",
+    "# !diff $data_path/20170809_NEMA_MUMAP_UCL.v.hdr umap.v.hdr"


This is probably quite interesting to participants who are familiar enough with diff

ashgillman · 2021-06-28T05:18:09Z

notebooks/PET/reconstruct_measured_data.ipynb

@@ -129,7 +137,7 @@
   "metadata": {},
   "outputs": [],
   "source": [
-    "!cp $SIRF_PATH/../STIR/scripts/IO/convertSiemensInterfileToSTIR.sh $SIRF_INSTALL_PATH/bin"
+    "# !cp $SIRF_PATH/../STIR/scripts/IO/convertSiemensInterfileToSTIR.sh $SIRF_INSTALL_PATH/bin"


Comment it out so that users with trigger-happy fingers don't run it unnecessarily (although it will be needed for the course)

interesting, is it going to fail on JupyterHub (and docker) due to read-only location? Better to write it in the current dir?

Hmmm.. I don't have JupyterHub account to test. Can you give me one?

But I think that can be a separate issue

ashgillman · 2021-06-28T05:18:55Z

notebooks/PET/reconstruct_measured_data.ipynb

+    "# is what we need to muliply by to correct for attenuation, while we computed the attenuation\n",
+    "# factors above, which is how much attenuation is estimated.\n",
+    "# Fortunately, these are simply the inverse.\n",
+    "acf_factors = attn_factors.get_uniform_copy()\n",


Not sure if readers will be familiar with the ACF/AF nomenclature?

KrisThielemans · 2021-06-28T06:00:03Z

notebooks/PET/MAPEM.ipynb

@@ -254,7 +254,7 @@
    "# Implement MAP-EM!\n",
    "Actually, we will implement MAP-OSEM as that's easy to do.\n",
    "\n",
-    "The lines below (almost) implement MAP-OSEM with a prior which just smooths along the horizontal direction. Either evaluate, and/or extend to 2D, and/or increase the speed of your Python code.\n",
+    "The lines below (almost) implement MAP-OSEM with a prior which just smooths along the horizontal direction.\n",


notebooks/PET/MAPEM.ipynb

notebooks/PET/reconstruct_measured_data.ipynb

KrisThielemans · 2021-06-28T06:12:06Z

notebooks/PET/reconstruct_measured_data.ipynb

+    "# This command prepends the data path to the data file so that the header in our working folder points to the data\n",
+    "# You likely won't need to do this for your own data if the data file is in the same directory.\n",
+    "!sed -i.bak2 -e \"s#\\(!name of data file:=\\)#\\\\1${data_path}/#\" umap.v.hdr\n",
+    "!sed -i.bak2 -e \"s#\\(!name of data file:=\\)#\\\\1${data_path}/#\" norm.v.hdr\n",


I'm assuming you checked it still worked ok?

Of course, they can be combined in1, but that probably doesn't help the user.

could add a note that this is needed due to limitations of the current convert* scripts in STIR if you think that'd help

KrisThielemans · 2021-06-28T06:13:33Z

notebooks/PET/reconstruct_measured_data.ipynb

@@ -129,7 +137,7 @@
   "metadata": {},
   "outputs": [],
   "source": [
-    "!cp $SIRF_PATH/../STIR/scripts/IO/convertSiemensInterfileToSTIR.sh $SIRF_INSTALL_PATH/bin"
+    "# !cp $SIRF_PATH/../STIR/scripts/IO/convertSiemensInterfileToSTIR.sh $SIRF_INSTALL_PATH/bin"


interesting, is it going to fail on JupyterHub (and docker) due to read-only location? Better to write it in the current dir?

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

KrisThielemans · 2021-06-28T07:23:59Z

done?

ashgillman · 2021-06-28T08:38:17Z

Yes, with two spin-off issues

Suggestions for MAPEM and reconstruct_measured_data

b12d298

ashgillman commented Jun 28, 2021

View reviewed changes

KrisThielemans requested changes Jun 28, 2021

View reviewed changes

This was referenced Jun 28, 2021

Does reconstruct_measured_data run on JupyterHub with the cp command #141

Closed

[1 1 1] kernel for MAPEM? #142

Open

Apply suggestions from code review

1fdc062

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

KrisThielemans merged commit b496149 into SyneRBI:master Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggestions for MAPEM and reconstruct_measured_data #140

Suggestions for MAPEM and reconstruct_measured_data #140

ashgillman commented Jun 28, 2021

ashgillman Jun 28, 2021

KrisThielemans Jun 28, 2021

ashgillman Jun 28, 2021

KrisThielemans Jun 28, 2021

ashgillman Jun 28, 2021

KrisThielemans Jun 28, 2021

ashgillman Jun 28, 2021

ashgillman Jun 28, 2021

ashgillman Jun 28, 2021

KrisThielemans Jun 28, 2021

ashgillman Jun 28, 2021

ashgillman Jun 28, 2021

ashgillman Jun 28, 2021

KrisThielemans Jun 28, 2021

KrisThielemans Jun 28, 2021

KrisThielemans Jun 28, 2021

KrisThielemans commented Jun 28, 2021

ashgillman commented Jun 28, 2021

Suggestions for MAPEM and reconstruct_measured_data #140

Suggestions for MAPEM and reconstruct_measured_data #140

Conversation

ashgillman commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Jun 28, 2021

ashgillman commented Jun 28, 2021