-
Notifications
You must be signed in to change notification settings - Fork 356
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
Support detector frame waveforms for inference #4686
Conversation
@mj-will Thanks for this PR, can you share the config file that you modified here? So I can test it, too. You mentioned you used your |
ee5f696
to
cab772e
Compare
@WuShichao sure, here you are. And yes, I started running with dynesty and after ~15 minutes it still had a dlogZ > 150 so I opted to use nessai instead for testing.
|
@mj-will This is still listed as a draft PR, would you like us to review it at this point? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mj-will I think this is a suitable straightforward implementation. Also thank you for adding errors for cases not implemented here.
Just confirm that this works in practice and I think a quick final look by @cdcapano would suffice here in case there's potential issues with this.
I think having an explicit switch of the generator is OK and as you said on the call will result in fewer mistakes. However, this is a bit inconsistent with what we've done in the heterodyne likelihood.
Should we try to make this consistent there now? E.g. there it 'automatically' detectors, but an explicit option would be just as good but would require updating the examples to reflect the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mj-will This looks good.
@ahnitz Yes, the relative binning model should be updated to reflect the changes here. But that's going to require changes to several lines of code, along with updates to the examples. To keep the PRs short and manageable, I think it best to merge this now, then fix relative binning to be consistent with it in a separate PR.
* add FD generator that uses fd_det_waveform * Fix time-shift for DirectDetFrameGenerator * add support for FDomainDirectDetFrameGenerator * add doc-string from det_frame_waveform
This PR adds support for using detector-frame waveforms (e.g. those produced by
get_fd_det_waveform
) in the inference module.Summary of changes
FDomainDirectDetFrameGenerator
det_frame_waveform
option to theGaussianNoise
class to which enables the use of the new generator.I considered automatically determining if a waveform is implemented in the detector frame or not, but I wasn't sure if a waveform could support both, so opted for a boolean instead. I can change this if it is preferred. I'm happy to rename the class/variables.
Example
My use-case for this is for performing inference in LISA with BBHx, so I've tested this with a modified version of the existing SMBH injection example. Here's the result:
I can also add this version to the directory if desired. I will note though that this is quite slow; running with nessai and 8 process, it takes around 10 minutes.
Other thoughts
I have not tested how this interacts with any other likelihoods, I know it won't be valid for some of them.