FEAT: add gwsignal waveform generator

[ColmTalbot]

@raffienficiaud this is a quick version of how I had envisioned an extension to the gwsignal support. It doesn't require explicit enumeration of all of the different kinds of source models, users can just do something like

import bilby

priors = bilby.gw.prior.BBHPriorDict()
priors["eccentricity"] = bilby.core.prior.Uniform(0, 1)
priors["mean_per_ano"] = 0.0
del priors["a_1"], priors["a_2"], priors["tilt_1"], priors["tilt_2"], priors["phi_jl"], priors["phi_12"]

wfg = bilby.gw.waveform_generator.GWSignalWaveformGenerator(
    waveform_arguments=dict(waveform_approximant="EccentricFD"),
    duration=4,
    sampling_frequency=1024,
    eccentric=True,
    spinning=False,
)

wfg.frequency_domain_strain(priors.sample())

I'm sure there are improvements that could be made to the interface, but I'm hopeful that something like this can be fairly transparent to the underlying gwsignal capabilities and ideally not need a lot of modification as more features are added to that. Since you have more experience with gwsignal I'd be glad to hear any thoughts.

Also @adivijaykumar @AntoniRamosBuades and @mj-will

[mj-will]

Thanks for doing things @ColmTalbot, it looks like a good start to me! I had a quick look and added some comments.

[lorenzopompili00]

Hi @ColmTalbot I also really like this idea!

Just want to mention that for bilby_tgr we also have a gwsignal_binary_black_hole function, which is a almost copy of the bilby one with just some extra lines here. To improve maintainability and reduce code duplication, it would be great to have an implementation that allows the waveform generator for bilby_tgr to be a subclass of the bilby one, without duplicating too much code as is done right now. I still need to look at this more closely, but I wanted to mention this as something to consider.

[raffienficiaud]

Thank you for the PR @ColmTalbot . I am all fine with any changes you make.
I have a questions about this interface: it relies on the fact that we can instanciate the GWSignalWaveformGenerator class with parameters: is this compatible with the way WF generators can be instanciated in the configuration of bilby_pipe?

[ColmTalbot]

Thank you for the PR @ColmTalbot . I am all fine with any changes you make. I have a questions about this interface: it relies on the fact that we can instanciate the GWSignalWaveformGenerator class with parameters: is this compatible with the way WF generators can be instanciated in the configuration of bilby_pipe?

That's a good question, it looks like it currently isn't possible. I see two options:

• add this as an option in a future release of bilby_pipe
• make the options here something that is passed through the waveform_kwargs

[raffienficiaud]

• add this as an option in a future release of bilby_pipe

I guess this is the way to go, but I don't know if we can make this ready by the freeze end of March (according to your email).

• make the options here something that is passed through the waveform_kwargs

For this, we would need a wrapper class that acts as a "factory" for the implementation suggested in the PR.

I don't know how hard is it to get the change into bilby_pipe, and I can give a hand. However, I would like in advance to check with the devs if end of March is something feasible from their perspective.

[ColmTalbot]

Reminder for me that I need to add some tests.

[ColmTalbot]

I noticed that the previous version was incompatible with parallelisation via multiprocessing due to a pickling issue. The current version will reconstruct the waveform generator every time it is pickled/unpickled. The way we handle multiprocessing, this means that the generator should only be set up once when the pool is created.

@raffienficiaud do you think this will cause any issues for SEOBNR?

[raffienficiaud]

I noticed that the previous version was incompatible with parallelisation via multiprocessing due to a pickling issue. The current version will reconstruct the waveform generator every time it is pickled/unpickled. The way we handle multiprocessing, this means that the generator should only be set up once when the pool is created.

@raffienficiaud do you think this will cause any issues for SEOBNR?

For pyseobnr, I do not think there is any issue in pickling. I cannot say anything about the GWsignal part.
This pickling is for multiprocess communication or it is also for disk storage? I am asking because the pickle files have reference to the python path from where the objects are defined, and this is usually an issue.

[ColmTalbot]

This pickling is for multiprocess communication or it is also for disk storage?

We do use them for both in bilby_pipe, but I think there's an assumption that the different stages in bilby_pipe use the same environment, so we should be safe there.

[raffienficiaud]

This pickling is for multiprocess communication or it is also for disk storage?

We do use them for both in bilby_pipe, but I think there's an assumption that the different stages in bilby_pipe use the same environment, so we should be safe there.

As long as there is no code change between the two, then it should be fine. However, if there is any reference to these objects in the pickled files (eg. posteriors) then this may be an issue.

I had a look at the code and now I am thinking that it is a bit of a pitty we would need to reinstanciate completely the generator for each subprocesses: there are some global objects that are being created the first time we load pyseobnr and this means we should recreate the cache for every subprocess. For a 64core node and for cache creation of 1minute, this means we waste 1hour compute time.

Do you have an intuition what is preventing the pickling? My guess is that there are some LAL objects laying around.

[ColmTalbot]

There are two things that can't be simply pickled.

• lal_dict: this can probably be worked around easily, I think we've also talked about implementing a serializer for this object in other contexts.
• seobnr: it looks like the generator stored the actual SEOB module can this be avoided?

[raffienficiaud]

There are two things that can't be simply pickled.

* `lal_dict`: this can probably be worked around easily, I think we've also talked about implementing a serializer for this object in other contexts.
* `seobnr`: it looks like the generator stored the [actual SEOB module](https://git.ligo.org/lscsoft/lalsuite/-/blob/master/lalsimulation/python/lalsimulation/gwsignal/models/pyseobnr_model.py?ref_type=heads#L32) can this be avoided?

Yes, I guess just storing the class GenerateWaveform should work as good, but I would not postpone the merge of the PR for this. The caching issue can be worked around in a different way.

[ColmTalbot]

I ran some tests and added an example using the new generator. Everything seems to be working well, so I think this is ready for a final round of review.

[raffienficiaud]

I ran some tests and added an example using the new generator. Everything seems to be working well, so I think this is ready for a final round of review.

Excellent, thank you @ColmTalbot

[mj-will]

This looks good to me. Just a couple of minor comments and questions.

[ColmTalbot]

Thanks for the comments, I'll address some of the longer ones later.

[mj-will]

LGTM!

[asb5468]

I've left some in-line comments for things I don't understand, but I think I'm missing the bigger context here. Why does this require an entirely new waveform generator and not just modifications to the source function? I don't love the fact that the kind of waveform generator now needs to be specified when the code is being set up, rather than just changing the source function.

[ColmTalbot]

I've left some in-line comments for things I don't understand, but I think I'm missing the bigger context here. Why does this require an entirely new waveform generator and not just modifications to the source function? I don't love the fact that the kind of waveform generator now needs to be specified when the code is being set up, rather than just changing the source function.

I'll start by saying that this is not a required change for users (unless they want to use eccentric waveform models without writing their own source functions) but the path that should require the least maintenance burden for supporting new waveform models.

There are a few advantages (ideally) to this method:

• on our end, we only need this one generator to cover all source classes and all permutations of spins/tides/eccentricity so there is less code needed for us. The alternative would be that we double the length of source.py to reimplement all of the different possible combinations of physics things.
• relatedly, if someone decides there are new parameters that are needed, those should be straightforward to add to our implementation in the same way that spins and tides are.
• part of the design motivation for the gwsignal generator is that the generators can cache things at initialization which would in principle speed up the per-evaluation cost, this hasn't been done in practice.

There are a few reasons that this method may not be always the preferred method:

• currently gwsignal doesn't support the frequency sequence methods that are needed for the accelerated likelihoods.
• there's some extra overhead associated with astropy.

[asb5468]

I'm happy with the latest updates and explanations.