search

CERN / TIGRE

TIGRE: Tomographic Iterative GPU-based Reconstruction Toolbox
BSD 3-Clause "New" or "Revised" License
550 stars 182 forks source link

Python algorithms tidy up #352

Closed yliu88au closed 2 years ago

yliu88au commented 2 years ago

Tidy up algorithms for Python: (1) SART_TV added (#272) (2) PCSD algorithm family added (3) FISTA algorithm extended to allow even faster convergence (FISTA_mod) (4) staticDetectorGeo added (#280)

The following total 18 algorithms for Python tested: 'FDK','SART','OSSART','SIRT','SART_TV','OSSART_TV','CGLS','FISTA', 'FISTA_mod','ASD_POCS','OS_ASD_POCS','AwASD_POCS','OS_AwASD_POCS', 'PCSD','OS_PCSD','AwPCSD','OS_AwPCSD','MLEM'

AnderBiguri commented 2 years ago

Before I do a review, there is a massive random number generator file, is it there by mistake?

yliu88au commented 2 years ago

I don't know where it come from, just deleted that.

yliu88au commented 2 years ago

Now FISTA and FISTA_mod is the same, but with different parameters. Also, I think, POCS family, including PCSD family, should be able to run as "OS" versions simply by specify blocksize = bsz, when bsz >1 when calling. All "OS" version is to allow blocksize = 20 as default.

AnderBiguri commented 2 years ago

Yes, the OS part is always something I wonder how to tackle. Historically and in some research circles, OS algorithms are treated very differently than per-projection algorithms. There is a point mathematically, as convergence properties are indeed different, however programatically, its basically just chosing a block size, and that will give you e.g. SART, OS-SART and SIRT.

So I am not sure what to do API-wise. It makes sense to have SART be able to do block sizes, but at the same time, SART is defined as to be 1 by 1. It gets muddier in ASD-POCS, PCSD etc, as the math of this does not require them to be projection by projection, but the articles that published them do published them as such, and many users are using them to compare with the articles. So, theoretically ASD-POCS with OS-SART update is still ASD-POCS (instead of OS-ASD-POCS, which I made up).

I think the best way, for consistency with publications and so people don't make mistakes is to to have PCSD be only bsz=1, and then have a OS_PCSD version, similar with the other algorithms

yliu88au commented 2 years ago

Yeah, that makes sense. Programmatically, it will be easy to define all algorithms in OS version as the starting class, then non OS version will simply inherit it with the blcoksize limited to 1. Anyway, it is a matter of choice.

AnderBiguri commented 2 years ago

Agree, as long as we give them separate names, and don't allow users to call non OS as OS, its all good.