TIGRE icon indicating copy to clipboard operation
TIGRE copied to clipboard

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

Open AnderBiguri opened this issue 3 years ago • 7 comments

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)

AnderBiguri avatar Apr 16 '21 11:04 AnderBiguri

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

Shubhamlmp avatar Apr 18 '21 06:04 Shubhamlmp

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.

AnderBiguri avatar Apr 19 '21 08:04 AnderBiguri

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

tsadakane avatar Aug 10 '21 09:08 tsadakane

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

AnderBiguri avatar Aug 10 '21 11:08 AnderBiguri

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 avatar Aug 10 '21 13:08 tsadakane

Oh, minimize => minimise :-P

tsadakane avatar Aug 10 '21 13:08 tsadakane

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

AnderBiguri avatar Aug 10 '21 14:08 AnderBiguri