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

Attempt to fix heterodyned likelihood #40

Closed
wants to merge 0 commits into from

Conversation

ThibeauWouters
Copy link
Collaborator

This PR attempts to fix the heterodyned likelihood, as raised in the issue #39

Specifically, I have edited the slicing of the waveforms in the following manner:

  • The previous version crashed when trying to slice the waveforms, since these are dictionaries. I have changed the code so that the slices are applied to both polarizations present in the dictionary.

  • The masks for slicing were created from the frequency grids. However, the previous version could result in a mismatch in size if the maximum of the valid frequency grid max(f_valid) is in between f_min and f_center of a particular bin of the relative binning. Now, I have created the masks based on the central frequencies grid, and sliced both the f_lower and f_center frequency grids of the heterodyned likelihood with this single mask.

  • A small edit changed the location where n_bins+1 was placed in the internal computations of the relative binning, as the old implementation was confusing me. This should not affect the execution of the code.

With these changes, the GW150914.py example script is able to run, thereby seemingly solving #39. However, I have not checked whether the obtained results make sense, as I first want to have this code reviewed by the original creators of jim.

@kazewong
Copy link
Owner

kazewong commented Nov 9, 2023

Hi Thibeau,

Thanks for contributing to jim. Indeed in the newest version of jim, refactoring of heterodyne likelihood is not completed yet so it could have some bug.

I am currently going through an application phase, so I am a bit behind on refactoring jim. I will have a look of this in approximately 2-3 weeks time.

@kazewong kazewong self-requested a review November 9, 2023 17:06
@ThibeauWouters
Copy link
Collaborator Author

I still found a bug in my own code, so I will have to review it again.

@kazewong
Copy link
Owner

You can keep this PR open and keep fixing your patch on the same branch, so one can see what have changed over time

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