Change the behaviour of XLALSimInspiralFD for precessing EOB

Description

This introduces changes to XLALSimInspiralFD only for time domain precessing EOB models: SEOBNRv3, SEOBNRv3_opt, SEOBNRv4P, SEOBNRv4PHM Essentially, the initial frequency is no longer modified internally by this function, which is the correct behaviour for these models.

API Changes and Justification

Backwards Compatible Changes

• This change introduces no API changes
• This change adds new API calls

Backwards Incompatible Changes

• This change modifies an existing API
• This change removes an existing API

If any of the Backwards Incompatible check boxes are ticked please provide a justification why this change is necessary and why it needs to be done in a backwards incompatible way.

Review Status

Please provide details on any reviews related to this change and and the associated reviewers.

@jolien-creighton Please take a look

added lalsimulation + 1 deleted label

@serguei.ossokine Should this use the spin frequency results for the waveforms given in XLALSimInspiralGetSpinFreqFromApproximant() instead of just hard-coding the list of waveforms to apply this to separately? In particular, shouldn't this also apply to SEOBNRv3_pert and SEOBNRv3_opt_rk4?

added 54 commits

• 8d4a7128...04b5382b - 53 commits from branch lscsoft:master
• b06fd68e - Change the behaviour of XLALSimInspiralFD for precessing EOB

Compare with previous version

@nathan-johnson-mcdaniel just pushed the change, please take a look. also @jolien-creighton

@serguei.ossokine Thanks--that looks good to me.

@serguei.ossokine, what's the status of this MR?

Noting as of 8 July 2020, the R&P group is requesting this change or an equivalent fix be put in place, in order to perform consistent precession BBH/NSBH injections over different pipelines for the end-O3 rate/population statements.

Hi, @riccardo-sturani Could you take a look at this and give your OK (as waveform review chair)? Thanks!

approved this merge request

Green light from my side.

Does this need to be rebased against current master?

added 701 commits

• b06fd68e...e6edc20d - 700 commits from branch lscsoft:master
• 195f9127 - Change the behaviour of XLALSimInspiralFD for precessing EOB

Compare with previous version

Looks like the pipeline failed in LALPulsar for some unrelated reason. @duncanmmacleod @adam-mercer could you take a look?

@patricia-schmidt @gregory.ashton @cjhaster please look over this MR as it might possibly affect Bilby PE.

This has been discussed in Bilby issues, see(bilby#308 (closed), bilby#443 (closed)).

I know that we had to address a time shift introduced by this function (bilby!736 (merged)). I thought that was the only remaining issue with using this function.

It sounds like this causes the output when using these EOB waveforms to be unexpected, but reproducible and understandable.

Hi folks,

Along with @Carl-Johan Haster and @Patricia Schmidt, we have no objections to this as I believe it improves the consistency of behaviour in SimInspiralFD.

I think there is a reasonable chance this could have an impact on bilby PE, especially the SEOBNRv4P and SEOBNRv4PHM runs on recent exceptional events. To investigate, I'll ask someone to run with the patch on either a recent event or injection and compare to runs without the patch. But, this I don't think this should hold up the MR as any unwanted effects will be due to the way Bilby interacts with SimInspiralFD rather than the patch itself (here I'm assuming it has been fully vetted within lalsuite itself).

mentioned in merge request !1375 (merged)

approved this merge request

resolved all threads

merged

mentioned in commit a3064e19

mentioned in issue #350 (closed)

mentioned in commit adam-mercer/lalsuite@a355b605

mentioned in commit adam-mercer/lalsuite@49b7087a