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.

Sign up for GitHub

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

Jump to bottom

Issue with injection parameters in run_sampler #606

Open
bilby-bot opened this issue Dec 3, 2020 · 2 comments · May be fixed by #900
Open

Issue with injection parameters in run_sampler #606

bilby-bot opened this issue Dec 3, 2020 · 2 comments · May be fixed by #900
Labels
bug Something isn't working conversion

Comments

@bilby-bot
Copy link
Collaborator

In GitLab by @git.ligo:gregory.ashton on Dec 3, 2020, 09:38

Email from Stephen Feeney:

It's nothing enormous, but it can result in the results.injection_parameter storing the wrong inclination angle, iota. This happens for me because I'm using a reference frequency that is different from bilby's default. In bilby/core/sampler/init.py, in run_sampler, when appending the injection_parameters to the result object, conversion_function is called without passing in the likelihood object (which in turn contains the user-specified minimum and reference frequencies). This means result.injection_parameters uses the default reference_frequency when converting theta_jn etc. into iota, and as a result my results objects' iotas don't match the ones actually used when injected signals. Niche :) But I thought you'd want to know :)

Pointing to the code, this line needs to also pass in the likelihood.

However, this creates a bit of a problem: if users pass in their own conversion functions which don't accept the likelihood it will fall over.

Fundamentally, I think we are missing a standard interface for the "conversion" function. I think the resolution here is to set a convention for the conversion function. What do folks think?
@bilby-bot
Copy link
Collaborator Author

In GitLab by @git.ligo:moritz.huebner on Dec 10, 2020, 12:58

Also pinging @git.ligo:colm.talbot

I don't know if I fully understand the issue and how to fix it since the conversion functions in bilby can be a bit confusing in how they work and what they do.

I know it is a bit hacky, but couldn't we just do a try/except clause around the line in run_sampler?

try:
   result.injection_parameters = conversion_function(result.injection_parameters, likelihood=likelihood)
except TypeError:
   result.injection_parameters = conversion_function(result.injection_parameters)

@bilby-bot
Copy link
Collaborator Author

In GitLab by @git.ligo:gregory.ashton on Dec 10, 2020, 13:41

This would be a temporary hack (to solve a minor niche problem). I think we are better off taking the time to make a consistent interface for the conversion function to fix this.

I'd also like to see more clarity around conversion/generation functions.

@ColmTalbot ColmTalbot linked a pull request Feb 3, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working conversion
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BUG: use likelihood for injection conversion function bilby-dev/bilby
1 participant
@bilby-bot