CERN / TIGRE

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

Put all regularization code in a `regularization.py` file, possibly a class #282

Open AnderBiguri opened 3 years ago

AnderBiguri commented 3 years ago

currently its mixed with their own files and part of iterative_recon_alg.py, and its a mess.

(more info is needed to clarify, please do let me know if you want to tackle this, I will add more info)

Shubhamlmp commented 3 years ago

Hello Dear @AnderBiguri I am interested in working on this issue and also on some good first issues. Thank You!

AnderBiguri commented 3 years ago

Hi @Shubhamlmp Great!

If you want to work on good first issues, maybe this one is not so much, as there are some technicalities to how the code is called that I myself don't know how to fix currently.

Instead, I suggest any of #284, #278 , #276, #270, #269, #268, #266, if that is of your interest. Happy to clarify anything required in any of those.

tsadakane commented 3 years ago

Quick question. The existing keyword is regularisation. Which name is appropriate, regularisation.py or regularization.py?

AnderBiguri commented 3 years ago

ahh well, english vs american spelling. I like the one with s more, but I guess that is because I am in Europe!

tsadakane commented 3 years ago

Understood.

What do you think about introducing regulariser class? The regularisation.py would be like

import numpy as np
from _minTV import minTV
from _AwminTV import AwminTV
from tigre.utilities.gpu import GpuIds

class regulariser:
    def __init__(self):
        pass

    def minimize(self, res_prev, dtvg, gpuids = None):
        return res_prev

## alternative of minimizeTV
class regulariser_tv(regulariser):
    def __init__(self, n_iter_tv):
        regulariser.__init__(self)
        self.n_iter_tv = n_iter_tv

    def minimize(self, res_prev, dtvg, gpuids = None):
        if gpuids is None:
            gpuids = GpuIds()
        return minTV(res_prev, dtvg, self.n_iter_tv, gpuids=gpuids)

## alternative of minimizeAwTV
class regulariser_awtv(regulariser):
    def __init__(self, n_iter_tv, delta):
        regulariser.__init__(self)
        self.n_iter_tv = n_iter_tv
        self.delta = delta

    def minimize(self, res_prev, dtvg, gpuids = None):
        if gpuids is None:
            gpuids = GpuIds()
        return AwminTV(res_prev, dtvg, self.n_iter_tv, self.delta, gpuids=gpuids)

and an algorithm object can have a member variable self.regulariser and calls minimize() from run_main_iter() if self.regulariser is not None. Some of the arguments of algorithm's init(), such as nitertv should be moved to regulariser. You don't have to modify IterativeReconAlg class to introduce Huber regulariser but add regulariser_huber to regularisation.py

I think this is an issue about the balance between the vanilla-ness and the object-oriented-programming which forces users to understand the classes before they use them.

What do you think?

tsadakane commented 3 years ago

Oh, minimize => minimise :-P

AnderBiguri commented 3 years ago

It does sound good actually.

I worry about about naming, not sure what I prefer. "regularisation" is a bit general, and the TV minimizer in minTV is not necessarily the only way to regularize an algorithm with TV... minTV just decreases with steepest descend the TV. However, other TV minimization uses proximals, and other math. The same with AwTV. So perhaps I prefer to leave those names as they are, for clarity