changed_apply_fd_time_shift_to_apply_fseries_time_shift_in_FDomainDetFrameTwoPolNoRespGenerator

[labani-01]

Correction to the "generate" function of FDomainDetFrameTwoPolNoRespGenerator - possible correction is to replace "apply_fd_time_shift" by "apply_fseries_time_shift" in the generate function of FDomainDetFrameTwoPolNoRespGenerator.

Marginalized_time model uses FDomainDetFrameTwoPolNoRespGenerator to generate the waveform. The "generate" function of FDomainDetFrameTwoPolNoRespGenerator uses "apply_fd_time_shift" for the time domain approximant to shift it by an amount tshift(1/df - abs(hp.epoch)). The goal of this step is to shift the merger at the boundary. But if we follow the source code of "apply_fd_time_shift" then it will be clear that it will shift the waveform by (shifttime - hp.epoch) amount.

Here shiftime = tshift and hp.epoch is negative, so (shifttime - hp.epoch) = (tshift - hp.epoch) = (1/df - abs(hp.epoch) - hp.epoch) = 1/df = length of the signal. So, the fucntion "apply_fd_time_shift" is not doing any shift to the signal and giving us the same time domain waveform.

To solve this problem we can use "apply_fseries_time_shift" function (which gives a relative shift). This function is giving the correct shift to the time domain waveform by an amount tshift = 1/df - abs(hp.epoch) and the merger is at the boundary.

Necessity of the the time-shift - pycbc inference models convert waveforms in frequency domain before calculating the likelihood. marginalized_time is one of the pycbc_inference models. Time marginalization is applied after generating the waveform (We integrate over tc). Each tc is just a time-shift of the waveform. The way this model incorporates different time-shifts is by doing inverse fourier transform of the inner product of signal and data at every discrete elements of every possible time shifts. We can only talk about the fourier transform at different time-shifts if there is some convention on the signal. A good likelihood value implies high value for the inner product between data and the signal. We know the standard reference in the signal is the location of the merger. The convention is to put the merger at the beginning of the vector then every time shift will give how much the signal needs to be shifted. Frequency domain waveform gives this setup by default. But for the time domain approximant we need to apply a time shift so that merger will be at the boundary or at the start of the signal vector.

[cdcapano]

If the tshift isn't being calculated correctly (I leave @ahnitz to verify). I think the correct fix would be to still call apply_fd_time_shift with a modified tshift rather than call apply_fseries_time_shift. apply_fseries assumes that the input array is equally spaced in frequency. That might always be the case here since this is used by the time marginalization model, but not sure. apply_fd_time_shift can handle both, albeit needing to take times rather than time shifts (something I regret doing, but would be too difficult to change now).

[ahnitz]

If the tshift isn't being calculated correctly (I leave @ahnitz to verify). I think the correct fix would be to still call apply_fd_time_shift with a modified tshift rather than call apply_fseries_time_shift. apply_fseries assumes that the input array is equally spaced in frequency. That might always be the case here since this is used by the time marginalization model, but not sure. apply_fd_time_shift can handle both, albeit needing to take times rather than time shifts (something I regret doing, but would be too difficult to change now).

It will be equally spaced actually, so I don't think that's a difference here.

[ahnitz]

@cdcapano I'll be out of contact, so can @cdcapano you review this after @labani-01 has updated it to address the issues.