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

BUG: use likelihood for injection conversion function #900

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

Conversation

ColmTalbot
Copy link
Collaborator

Resolves #606

There has been a very long-standing issue where the generate_all_cbc_parameter functions didn't use the correct waveform arguments for injections. I think will resolve that in a generic way as long as the conversion function extracts that information from the likelihood. This now mirrors how the call is made for the posterior samples.

@mj-will
Copy link
Collaborator

mj-will commented Feb 4, 2025

Would this mean we also need to update identity_map_conversion here?

It might not cause issues immediately, but I think it would break bilby_pipe once/if this MR is merged.

@ColmTalbot
Copy link
Collaborator Author

I don't think so, this is the "generation" function, so this is the relevant function.

I just updated the docstring to specify this.

@mj-will
Copy link
Collaborator

mj-will commented Feb 4, 2025

I don't think so, this is the "generation" function, so this is the relevant function.

I just updated the docstring to specify this.

Ah, I'd missed the there are two functions. Thanks for pointing that out.

@mj-will mj-will added the bug Something isn't working label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with injection parameters in run_sampler
3 participants