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

Time and phase marginalization #71

Merged
merged 11 commits into from
Feb 22, 2024

Conversation

tsunhopang
Copy link
Collaborator

This commit is in response to the issue #2

You may find the comparison corner plots as follows:

corner_plot_intrinsic_comparison
corner_plot_extrinsic_comparison

@tsunhopang
Copy link
Collaborator Author

@kazewong this git conflict might need your intervene

@kazewong kazewong self-requested a review February 20, 2024 15:06
@kazewong
Copy link
Owner

I fixed the conflicts and moved the internal likelihood function into likelihood.py. I also add typing information in the function.

Before merging this, I think it will be beneficial to also migrate the body of the heterodyne likelihood to the same API.

@tsunhopang
Copy link
Collaborator Author

Yes, that was the plan to have it also included there in another pull request. I can have it also add it there within this one.

For relative binning, only phase marginalization will be added; as the time marginalization will then use a non-uniform grid fft, for which I am not sure if it is beneficial (e.g. see Appendix A in https://arxiv.org/abs/2312.06009)

@kazewong
Copy link
Owner

If it is not a lot of changes, perhaps we can just have it in this PR.

On relative binning, I agree we can first add phase marginalization.

For time marginalization, I think at some point we can consider using a non-uniform fft package jax-finufft, which to my understand is quite performant. The main issue there currently is the installation will get a bit more complicated than simple pip install, and asaik the API and functionality is not necessarily stable yet. So this is more likely to go into future PR

@tsunhopang
Copy link
Collaborator Author

tsunhopang commented Feb 20, 2024

For the relative binning likelihood; the optimizer finds the following parameters;

Injected parameters = {
'M_c': 28.095555795460438,
'eta': 0.24710059171597634,
's1_z': -0.016246134632590996,
's2_z': -0.04423844417344978,
'd_L': 445.4687493344804,
't_c': 0.0,
'phase_c': 4.100547786091415,
'iota': 2.920749029724262,
'psi': 0.787166765060988,
'ra': 0.15039344639787625,
'dec': -0.5355555460283846
}
ref_parameters_original = {
'M_c': 28.125974539678566,
'eta': 0.25,
's1_z': -0.99,
's2_z': 0.9191400868807302,
'd_L': 223.59820615113975,
't_c': 0.009947852095418,
'phase_c': 6.283173520274804,
'iota': 1.3616503037593668,
'psi': 3.141298463313976,
'ra': 2.2766191364329975,
'dec': 1.1116867914351234
}
ref_parameters_margphi = {
'M_c': 28.13080784160065,
'eta': 0.24999999999599654,
's1_z': -0.99,
's2_z': 0.9200876980166237,
'd_L': 223.01512154400882,
't_c': 0.010463400533415948,
'iota': 1.3685230276718776,
'psi': 1.5009806917550423,
'ra': 2.202961964586829,
'dec': 1.1170486830648356
}

I was expecting the marginalization over phase would help the optimizer to get closer to the true parameters, yet, the impact is minimal.

For PE, I have two sets of runs with both the reference parameters fixed at the injected parameters, so that we have an apple-to-apple comparison; the corner plots are as follows

corner_plot_intrinsic_comparison
corner_plot_extrinsic_comparison

return jnp.log(i0e(x)) + x


def original_likelihood(params, h_sky, detectors, freqs, align_time, **kwargs):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to put the likelihood function in the likelihood.py file instead of utils.py. You can leave the logi0 in.

@kazewong kazewong merged commit df50731 into kazewong:main Feb 22, 2024
6 of 7 checks passed
@tsunhopang tsunhopang deleted the time-phase-marginalization branch February 22, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants