Re: [SyneRBI/SIRF] stir.make_Poisson_loglikelihood seems to ignore the model parameter (#567)

[Kris Thielemans]

@evgueni-ovtchinnikov can you look at this one?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[Evgueni Ovtchinnikov]

@KrisThielemans: So far as I can see from these two lines:

def make_Poisson_loglikelihood(acq_data, model = 'LinearModelForMean'):
...
    obj_fun = PoissonLogLikelihoodWithLinearModelForMeanAndProjData()
                                      ~~~~~~~~~~~~~~~~~~

model argument does not specify the acquisition model. My guess is, it specifies particular Poisson LogLikelihood model - cannot really say anything more useful, I am afraid, being out of my depth here.

[Richard Brown]

Seems there are two different meanings of model here. One being the AcquisitionModel (which is what @francescaleek expected), the other being the "model" used for the likelihood (e.g., LinearModelForMean). Model seems like a confusing name for the latter, we should maybe change it (e.g., likelihood_type).

@evgueni-ovtchinnikov perhaps an error should be thrown here?

def make_Poisson_loglikelihood(acq_data, likelihood_type = 'LinearModelForMean'):
    if likelihood_type=='LinearModelForMean':
        obj_fun = PoissonLogLikelihoodWithLinearModelForMeanAndProjData()
    else:
       raise error("unknown likelihood type, here's a list of possible entries...")

[Richard Brown]

For @francescaleek 's desired functionality, we would also need:

def make_Poisson_loglikelihood(acq_data, likelihood_type = 'LinearModelForMean', model=None):
    if likelihood_type=='LinearModelForMean':
        obj_fun = PoissonLogLikelihoodWithLinearModelForMeanAndProjData()
    else:
       raise error("unknown likelihood type, here's a list of possible entries...")

    if model is not None:
        obj_fun.set_acquisition_model(model)

[Evgueni Ovtchinnikov]

Closed #567 via 89620f9.

[Kris Thielemans]

Reopened #567.

[Kris Thielemans]

I thinl @rijobro's suggestion is very good. The intention was that we'd be able to figure out the likelihood type from the model anyway (after all, we only need to know what the domain and range are). So I even think that the likelihood_type will become irrelevant.

[Kris Thielemans]

let's use acq_model as parameter name

[Evgueni Ovtchinnikov]

Closed #567 via fd57301.