Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Everywhere for standard Likelihood #381

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

thjsal
Copy link
Contributor

@thjsal thjsal commented Apr 26, 2024

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 and others added 3 commits April 26, 2024 14:17
The default imaging extension has been changed from ``uniform.pyx`` to ``PDT_U.pyx`` to avoid the need for X-PSI re-installation when computing neutron star images for different nested models. The tutorials have been updated to account for this change when necessary. Some bugs in the imaging function were also fixed, and the Global surface emission -tutorial was added back to the documentation pages.
@thjsal thjsal linked an issue Apr 26, 2024 that may be closed by this pull request
@thjsal thjsal requested a review from yveskini April 26, 2024 21:09
@thjsal thjsal added the bugfix label Apr 26, 2024
@thjsal
Copy link
Contributor Author

thjsal commented May 3, 2024

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
Copy link
Contributor Author

thjsal commented May 7, 2024

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 and others added 3 commits May 8, 2024 00:26
@thjsal
Copy link
Contributor Author

thjsal commented May 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Likelihood does not support Everywhere
1 participant