Discrepancy in definitions of matched filter SNR

On issue #221 (closed), @cjhaster wrote

In LALInference the matched_filter_snr isn't defined as sqrt(Re{<d|h>}), but instead as Re{<d|h>}/sqrt(Re{<h|h>}). See LALInference reference here, and in surrounding lines

(I'm opening a new issue as the original issue was a purely programatic error I think).

Should we change the definition in bilby to match that of lalinference?

I would argue so. Having the MatchedFilterSNR being able to go negative is a feature and not a bug At the same time, it's also useful to save all info available from the <d|h> inner product (either as a Re-Im pair, or as a Amp-phase pair). This is good to have around when diagnosing potential weirdnesses in runs.

I would vote for the LALinference definition - is the other one even right? has someone checked it for known events? - I just had some old code lying around (no guarantees it is correct) and don't get the same answer for a random waveform, as you would expect, (where d is signal + gaussian noise and h is template). If I'm being stupid someone please tell me as I am definitely not an expert in these things.

If the former (currently in Bilby) is right and is just a different notation/definition thing, its rather weird notation and does not make sense to me and it may potentially confuse people, (certainly confuses me) who have followed classic texts like Wainstein and Zubakov (1970) or Cutler and Flanagan (1994) when deriving the matched-filter/optimal SNR.

The code is doing what it is supposed to be doing (at least in terms of how the likelihood is calculated), but the variables names are misleading and should be changed.

The log likelihood can be written

logl(L) = log(ZN) + x - (1/2)*rho_opt^2

where

x = <mu,d> .

Currently, the code refers to the quantity denoted x as the matched filter SNR squared. However, as @cjhaster notes, and @sylvia.biscoveanu points out,

x ≠ rho_mf^2 .

Rather,

x = rho_mf * rho_opt

I suggest

• Change the name of the variable denoted here as x to something other than matched filter signal-to-noise ratio. We can even call it x if people want.
• Add some comments to the code indicating that x = rho_mf * rho_opt.

@colm.talbot

I think the issue is that when one calls bilby.gw.utils.matched_filter_snr_squared, that function returns what @eric.thrane calls x, and not the MF SNR as per LALInference's definition, which is misleading. There should be a function called matched_filter_snr_squared that returns the MF SNR even if we relabel the current function as x.

created branch 255-discrepancy-in-definitions-of-matched-filter-snr

mentioned in merge request !306 (merged)

I created an MR to address this, !306 (merged). For want of a better name I called what was previously matched_filter_snr_squared d_inner_h.

That works, @colm.talbot. Is there a new, corrected version of matched_filter_snr folks can use, e.g., for selection effects?

There is indeed.

closed via merge request !306 (merged)

mentioned in commit 62d036ee