-
Notifications
You must be signed in to change notification settings - Fork 23
Distribute pastro #404
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
Distribute pastro #404
Conversation
|
Should we just move the entire p astro logic to the amplfi subprocess? |
|
Probably, yeah. Let me try that. |
|
I take it back, the reason to keep them separate is that we want the p_astro calculation and submission to run in parallel with the skymap creation submission to save time. |
|
Okay got it - so then we should probably just trigger the pastro process (i.e. add to the pastro queue) from the amplfi process after the sampling is done |
|
The way we have things set up makes that a little non-trivial to do without losing time. We'd either have to pass the samples between subprocesses, or wait until the file gets written in |
deepchatterjeeligo
left a comment
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.
The current PR will be a functional implementation. There can be corner cases, which may make us rethink the order of the sub-processes. But this looks good to me as a first stab.
|
@wbenoit26 Why are we using detector frame masses? |
|
@deepchatterjeeligo @EthanMarx With the config parameters corrected, the p_nsbh values that we get out of this look good. If one of you can review and approve, I'll merge this in, make a release, and start running over the LLPIC live |
See @deepchatterjeeligo's comment above |
|
@EthanMarx @deepchatterjeeligo Could one of you review and approve? |
|
Im confused why we're using detector frame masses? The search pipelines can get source frame masses, the templates measure a distance and then after assuming a cosmology you get a redshift |
No because the template have an effective distance, not the luminosity distance that is needed to back out the redshift. |
|
Regardless, we are able to get the source frame mass so why would we not use it? This is exactly what bilby does when they update the p_astro. |
deepchatterjeeligo
left a comment
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.
Looks good to me. I understand this is difficult to test aside from a live environment. Has that been done, on the O3 MDC maybe? If not that's OK, happy to merge this and sort out the kinks.
|
@EthanMarx that's correct, but it is a separate discussion. Our goal with this is to be compatible with the definition of the p-astro from other CBC pipelines. |
|
I've tested with the offline-online code, which should match all the online functionality. But I think it would be good to test on the O3 MDC data for a little while and make sure that everything goes through. I can do that today. |
|
I think the source frame vs det frame p-astro is a good point to ask allsky about. Obviously, the better definition is the source frame version. |
|
@deepchatterjeeligo I asked at the all-sky call this morning, and it sounds like the matched filtering pipelines have some kind of method to reconstruct the redshift from the effective distance, so they do their p_astro calculation with source frame masses. I'm going to switch back to using source frame masses here. |
|
I've been running over the O3 MDC today and haven't encountered any issues. Merging this and making a release that I'll start on LLPIC tomorrow. |
@deepchatterjeeligo Is this what you had in mind for distributing p_astro across the categories?