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

PyGRB: Re-write injection infrastructure #3468

Closed
a-r-williamson opened this issue Sep 24, 2020 · 14 comments
Closed

PyGRB: Re-write injection infrastructure #3468

a-r-williamson opened this issue Sep 24, 2020 · 14 comments
Assignees
Labels
PyGRB PyGRB development work in progress

Comments

@a-r-williamson
Copy link
Contributor

PyGRB injections should use hdf5 and generally not be so ad hoc.

This entails replacing lalapps_inspinj, pycbc_dark_vs_bright_injections , pycbc_split_inspinj, and the pylal codes with one executable that makes suitable injections for a GRB search.

This can potentially piggy back on pycbc_create_injections and pycbc_hdf5_splitbank.

@a-r-williamson a-r-williamson added the PyGRB PyGRB development label Sep 24, 2020
@a-r-williamson a-r-williamson added this to the PyGRB - Phase 1 milestone Sep 24, 2020
@titodalcanton
Copy link
Contributor

@a-r-williamson you may want to watch out for the issue described in #3427.

@spxiwh
Copy link
Contributor

spxiwh commented Sep 24, 2020

It would be a great test of the new injection infrastructure if you can directly sample from the required GRB distributions with create_injections and a suitable config file!

@pannarale
Copy link
Contributor

pycbc_create_injections ---> pycbc_dark_vs_bright_injections ---> pycbc_hdf5_splitbank. So I think the first thing to do is to enable hdf5 stuff in pycbc_dark_vs_bright_injections

@ahnitz
Copy link
Member

ahnitz commented Sep 24, 2020

@a-r-williamson The current limitation of pycbc_create_injections (and some of the stuff below) is what Tito has pointed out. At the moment that means it is difficult to do an injection set that uses multiple waveform approximants at the same time. The other issue is is how to place injections in time. As is, there isn't a way to ensure minimum separation between injections. I am working to address that in #3415, but haven't gotten back to addressing the reviewer comments yet.

@spxiwh
Copy link
Contributor

spxiwh commented Sep 24, 2020

The idea of the new stuff @cdcapano wrote is to avoid having to need codes like pycbc_dark_vs_bright_injections and instead have this all encoded in the sampling parameters. So it should be possible to get this to do:
pycbc_create_injections ---> pycbc_hdf5_splitbank

(This would have the added benefit of having dark_vs_bright directly available for PE sampling!)

@pannarale
Copy link
Contributor

pannarale commented Sep 24, 2020

So, is the suggestion to do this via the constraints attribute of pycbc.distributions?

@pannarale
Copy link
Contributor

pannarale commented Sep 24, 2020

From pycbc_create_injections: "The parameter that constraints are applied to may be any parameter in
variable_params or any output parameter of the transforms. Functions may be applied on these parameters to obtain constraints on derived parameters. " So it's a matter of having the remnant mass as a derived parameter and then asking for it to be >= 0.

@cdcapano
Copy link
Contributor

@pannarale Yes, that's right. To do that, you need a function in pycbc.conversions that calculates the remnant mass. Then you can use that function as a constraint.

What is the function you are looking to use? Is it just an estimate of the final mass from NR? We do currently have final_mass_from_initial which uses the fit in SEOBNR. Or is it a different function? If so, just add it to conversions and file a PR for it. Note that the standard in conversions is to name the function {output}_from_{inputs}, and that it only return one parameter at a time. Anyway, as an example of what this would look like using the final_mass_from_initial, you would add the following to the config file for create_injections:

[constraint-final_mass]
name = custom
constraint_arg = final_mass_from_initial(mass1, mass2, spin1z=spin1z, spin2z=spin2z) > 0

(this assumes you have variable params mass1, mass2, spin1z, spin2z.)

@spxiwh
Copy link
Contributor

spxiwh commented Sep 24, 2020

@pannarale Yes, I think so (but @cdcapano is the expert, so feel free to consult him in Slack). This issues Alex and Tito raise should be fixed in the next ~month as these are needed for all-sky search (but of course if one of you lot beat us to it, then great!)

@cdcapano
Copy link
Contributor

@titodalcanton Sorry, I forgot about that issue you rose. I'll get back to it.

@spxiwh
Copy link
Contributor

spxiwh commented Sep 24, 2020

I suggest to move discussion of the constraint to the Slack.

@a-r-williamson
Copy link
Contributor Author

@spxiwh @pannarale @ahnitz @sfairhur

@SamuelH-97 and I will be looking at the EM-bright constraint. We have some relevant notes sketched out here but are yet to make any real code changes for a PR.

One thing we wanted to ask for views on is: how much of the code in pycbc/tmpltbank/em_progenitors.py should we port elsewhere, including where to put the data file in pycbc/tmpltbank/ns_sequences? Does it make sense for this to be under tmpltbank going forward?

@spxiwh
Copy link
Contributor

spxiwh commented Nov 10, 2021

@a-r-williamson The EMbright stuff doesn't really belong in tmpltbank but it was perhaps easiest to put it there while it wasn't used anywhere else. Please feel free to move it elsewhere. Although if the datafile risks getting large (at the moment it is tiny) we should consider having it somewhere outside of PyCBC.

@titodalcanton
Copy link
Contributor

@pannarale do you consider this issue resolved now?

@github-project-automation github-project-automation bot moved this from In Progress to Done in PyGRB Development Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyGRB PyGRB development work in progress
Projects
Status: Done
Development

No branches or pull requests

8 participants