For those without Python IDE, it looks nicer on

https://github.com/CCPPETMR/xSTIR/blob/upstream_update/examples/osmaposl_reconstruction.py

Also, might be best to check that link as opposed to Evgueni’s attachment, in case we're changing it as we go...

As discussed during the meeting, I think we need a separate document (relatively brief) that describes the library (essentially based on the current pseudo-code document, but removing some/all of the examples and adding info on file formats, and ideally on how to get data from the scanners accordingly (we'll need to check with GE/Siemens what we can include there).

Some comments:

- I would use a docstring at the top

- I wouldn't give the engine option. Right now, we don't support another engine, so it will just confuse the user (we could have a separate demo with engine selection, clearly labelling it as for the future). Also, it'd allow us to get rid of all the parsing stuff which most people will not understand. (for other demos, we agreed to use docopt for the parsing in Python).

- wouldn't you need to import pylab?

- Need to provide some lines for definition of “main()” (why we do it)

- I'd like to get an easier way to redirect the warnings and info. Could we have something like redirect_diagnostics_to_files('INFO.txt', 'WARN.txt') ? (Not sure about life-time of the printerTo objects)

- as discussed, we'll need to rename 'my_forward_projection.*s'. Maybe 'simulated_PET_data.*s'? (will need to adapt .hs file)

- I would delete the text "(which contains forward projection of the actual image stored in file my_image.hv - see below)" It is confusing and unnecessary (the demo will work with any acquisition data).

- Change "indicate that acquisition data source is this file" to "Read PET data from a file. Supported file formats are discussed elsewhere".

- for ad.create_empty_image, I would add something like "This will choose default voxel sizes. There are options available to change these, see demo XXX" (or "do help ad.create_empty_image", or "check the document that describes the library").

- I think we should rename PoissonLogLh_LinModMean_AcqMod to PoissonLogLikelihood. I’m not 100% sure if we can let it figure out of the acquisition model is non-linear (e.g. for kinetics), and if the data is “AcquisitionData” (and not listmode), but I think we can.

- For OSMAPOSLReconstruction, add “As we are not using a penalty (or prior) in this example, this will actually run OSEM”

- For set_num_subsets(), add a comment “Set the number of subsets (note that this has to be a divisor of the number of azimuthal angles”. In actual fact, in current STIR this is more complicated as it depends on symmetries used… that’s for another email I guess

- Need to say what set_up() does (i.e. provide initial image (or maybe not?), and tell reconstruction to use same voxel-size etc. it will also compute the subset-sensitivity images in current STIR).

- I think I’d remove the bit about “compare to actual image”. This assumes that the demo works with forward projected data, but it doesn’t need to.

Kris

-----Original Message-----
From: [log in to unmask] [mailto:[log in to unmask]]
Sent: 02 December 2016 13:06
To: Thielemans, Kris <[log in to unmask]>; [log in to unmask]
Subject: RE: demo script - please comment

Dear All,

Attached is my best so far attempt at a well-documented demo script.

Your corrections and suggestions for improvement sent to me not later than Monday will be highly appreciated.

Thank you,

Evgueni