PyGRB: Re-write injection infrastructure
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 you may want to watch out for the issue described in #3427.
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!
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
@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 https://github.com/gwastro/pycbc/pull/3415, but haven't gotten back to addressing the reviewer comments yet.
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!)
So, is the suggestion to do this via the constraints attribute of pycbc.distributions?
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.
@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.)
@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!)
@titodalcanton Sorry, I forgot about that issue you rose. I'll get back to it.
I suggest to move discussion of the constraint to the Slack.
@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?
@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.
@pannarale do you consider this issue resolved now?