search

CERN / TIGRE

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

Offset redundancy weighting #376

Closed grupesh closed 2 years ago

grupesh commented 2 years ago

Added data redundancy weighting necessary for offset detector geometry (as an option) in all algorithms, except MLEM, tested with simulation & real data.. also includes

AnderBiguri commented 2 years ago

Thanks a lot!

However, the optional parameter should come in as part of varargin as a keyword argument, similar to all the rest of the optional parameters. Can you please put the code there? Let me know if this needs clarifying.

grupesh commented 2 years ago

Will do this in a bit

grupesh commented 2 years ago

changed redundancy weighting to be part of "varargin". Please review and merge

AnderBiguri commented 2 years ago

Hi @grupesh ,

Can I make commits to this branch? I am happy to tackle the reviews myself to add it to TIGRE.

grupesh commented 2 years ago

Sure sure, sorry about the delays on my end..

AnderBiguri commented 2 years ago

@grupesh absolutely no worries! This is a free contribution to an open source software! Thanks a lot !

AnderBiguri commented 2 years ago

Final question/comment: in the wang weigthts in FDK, they are multiplied by 2 in the end. Is there any reason you have not done this in your reundacy_weighting() function? just forgot, or is it better without it?

grupesh commented 2 years ago

I've kept multiplication by 2 in redundancy weighting, same as preweighting2. I think multiplication by 2 is helpful/necessary, in the definition of weights in Wang paper, multiplication factor of 2 is present.

Best, Rupesh

On Thu, Aug 4, 2022, 06:13 Biguri @.***> wrote:

Final question/comment: in the wang weigthts in FDK, they are multiplied by 2 in the end. Is there any reason you have not done this in your reundacy_weighting() function? just forgot, or is it better without it?

— Reply to this email directly, view it on GitHub https://github.com/CERN/TIGRE/pull/376#issuecomment-1205049356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AORSFGDEONVRRPVJTJG5C6LVXOJVVANCNFSM5ZCVVKKQ . You are receiving this because you were mentioned.Message ID: @.***>

AnderBiguri commented 2 years ago

Where is the multiplicatioin happening? I can't see it

AnderBiguri commented 2 years ago

I think this is ready to merge. If you think so too, I will!

Thanks again for the great work.

grupesh commented 2 years ago

Yes, I had forgotten the multiplication by 2 factor, thanks for taking care of that and cleaning up the code. I think it's ready to merge.

AnderBiguri commented 2 years ago

Thanks to you for the hard work @grupesh