Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changed_apply_fd_time_shift_to_apply_fseries_time_shift_in_FDomainDetFrameTwoPolNoRespGenerator #4825

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

labani-01
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

ahnitz commented Jul 26, 2024

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
Copy link
Member

ahnitz commented Aug 4, 2024

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

{
"cells": [],
"metadata": {},
"nbformat": 4,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful not to commit files you don't intend to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't change the generate.py script (keeping apply_fd_time_shift in the FDomainDetFrameTwoPolNoRespGenerator) and want to get the time domain waveform using a time domain approximant 'SEOBNRv4', then it will give the plot below.

params={'mass1' : 36.09431175153813,
'mass2' : 33.61286267925508,
'distance' :  500.60060060060056} 
from pycbc.waveform.generator import FDomainDetFrameTwoPolNoRespGenerator
from pycbc.waveform.generator import TDomainCBCGenerator
generator = FDomainDetFrameTwoPolNoRespGenerator(TDomainCBCGenerator, 0, variable_args=['mass1', 'mass2', 'distance'],  delta_f=1./68, delta_t=1/2048, f_lower=20., approximant='SEOBNRv4')
no_det = generator.generate(**params) #frequency domain waveform
print(no_det['RF'])
plt.plot(no_det['RF'][0])
plt.show()

add_fd_frequency

plt.plot(no_det['RF'][0].to_timeseries())
plt.show()

add_fd_timedomain
If we notice the time domain plot then it is clear that merger is not shifted at the beginning of the waveform or the merger is not at the boundary.

But if we do the required changes (changing apply_fd_time_shift to apply_fseries_time_shift in the FDomainDetFrameTwoPolNoRespGenerator) then it is giving the expected nature of the time domain plot.

add_fseries_frequency - this is the frequency domain waveform after the change.

add_fseries_timedomain

  • this is the expected time domain plot which we are getting after doing the above mentioned change.

Copy link
Member

@ahnitz ahnitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@labani-01 Thanks. I think this looks good now. Assuming the tests pass, this may now be merged.

@ahnitz ahnitz enabled auto-merge (squash) September 9, 2024 20:35
@ahnitz ahnitz merged commit 32030e0 into gwastro:master Sep 9, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants