search

SyneRBI / SIRF

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

Document the required variables for stir.AcquisitionData constructor for use with NiftyPET #612

Open AnderBiguri opened 4 years ago

AnderBiguri commented 4 years ago

Add it at least to acquisition_data_from_scanner_info.py, and any other place where info about niftyPET is available. Only this constructor works for NiftyPET: AcquisitionData('Siemens_mMR',span=11,max_ring_diff=60)

KrisThielemans commented 4 years ago

I don't know where you want to put this. As I wrote in the email on the devel list, AcquisitionData doesn't (and shouldn't) know about NiftyPET.

AnderBiguri commented 4 years ago

@KrisThielemans but the user should, right? This is just an issue to add some text, somewhere in the examples, to document which are the required parameters if GPU code wants to be used.

KrisThielemans commented 4 years ago

agreed, but the question is where it needs to be documented... could indeed say in acquisition_data_from_scanner_info.py (and .m) something like "By default, the Siemens mMR uses acquisition settings corresponding to the following constructor". I'm afraid I don't really like mentioning NiftyPET in there, as 1) it isn't merged yet 2) it is probably a limitation of @rijobro 's NiftyPET wrapper 3) it creates dependencies.

of course, if there's a NiftyPET demo, then we put it there as well

AnderBiguri commented 4 years ago

That makes sense @KrisThielemans . Maybe we can leave this open until @rijobro's PR is merged?

KrisThielemans commented 4 years ago

Could just do the current demo already

AnderBiguri commented 4 years ago

@KrisThielemans certainly, I meant keep it open even after current needed modifications are put (the demo) , until the PR is merged.