xpsi-group / xpsi

X-PSI: X-ray Pulse Simulation and Inference
Other
34 stars 21 forks source link

Allow Everywhere for standard Likelihood #381

Closed thjsal closed 3 months ago

thjsal commented 4 months ago

Likelihood.py modified so that it can be used for both HotRegion and Everywhere objects without user customization. In case of Everwhere, however, one needs to make a CustomEverywhere where an extra phase_shift parameter is added like this (EDIT: THIS IS NOT NEEDED ANYMORE):

class CustomEverywhere(xpsi.Everywhere):
    """ Custom surface radiation field globally spanning the surface. """

    @classmethod
    def create_parameters(cls, bounds, values={}, *args, **kwargs): 
            ......
             phase_shift = Parameter('phase_shift',
                                      strict_bounds = (-np.infty, np.infty),
                                      bounds = bounds.get('phase_shift',None),
                                      doc = "The phase of the everywhere [cycles]",
                                      symbol = r'$\phi$',
                                      value = values.get('phase_shift',None))

              return cls(*args,
                         custom=[...,
                                 phase_shift],
                         **kwargs)

This parameter can be fixed to zero when creating an Everywhere instance:

values = dict(phase_shift = 0.0)
everywhere = CustomEverywhere.create_parameters(bounds=...,
                                                values=values,
                                                ...)

This is to avoid having extra if or try statement in the likelihood evaluation slowing the inference runs (an option disfavored by @yveskini), but still being able to use the same Likelihood syntax for both HotRegions and Everywhere.

We can add phase_shift to be also part of Everywhere class by default, but maybe it is not needed, since for usual applications that class is customized anyway?

thjsal commented 4 months ago

I just noticed that for some modules we have used this update produces this error: AttributeError: 'CustomSignal' object has no attribute 'surf'

I don't know why this occurs only for some of the tests (e.g. TestRun_BB.py works but TestRun_PolNum_split_inference.py does not). But need to still fix this issue.

thjsal commented 4 months ago

I found that the issue occured always when having multiple signal objects.I fixed it by changing signal.surf to photosphere.surf, since it actually makes more sense to save surface model (which is either photosphere.hot or photosphere.everywhere) to the photosphere class. It is also more consistent with the original code which used always photosphere.hot.

thjsal commented 3 months ago

Now I also modified the code so that Everywhere class will always create and fix this additional phase_shift parameter to zero without any customization from the user.

thjsal commented 3 months ago

I made changes to the coding style following mostly the suggestions of @yveskini. In addition, I moved setting the correct photosphere.surface from the init function of Likelihood.py to the init function of Photosphere.py, since I see no reason why it could not just happen there already.

I tested that the new code also works for the basic examples with HotRegion and Everywhere classes. I also tried to check the do_fastoption, but that option seems to be broken even without the changes in this pull request.