search

gwastro / pycbc

Core package to analyze gravitational-wave data, find signals, and study their parameters. This package was used in the first direct detection of gravitational waves (GW150914), and is used in the ongoing analysis of LIGO/Virgo data.
http://pycbc.org
GNU General Public License v3.0
307 stars 344 forks source link

Fix epoch in cached fft #4766

Closed mj-will closed 1 month ago

mj-will commented 1 month ago

Update pycbc.strain.strain.execute_cached_fft so that the epoch of the output frequency/time series is set to match that of the input rather than being zero.

Standard information about the request

This is a: bug fix

This change affects any code that makes use of execute_cached_fft or functions/classes that call it. This includes pycbc.filter and pycbc.psd given this, it could impact a large part of the codebase.

This change changes: scientific output

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

This change will: fix an existing bug

Motivation

This fixes a bug in pycbc.strain.strain.execute_cached_fft

Contents

Links to any issues or associated PRs

N/A

Testing performed

Added unit tests to test the changes

Additional notes

Minimal example

import pycbc
import pycbc.strain
ts = pycbc.types.TimeSeries([1, 2, 3], delta_t=0.2, epoch=1e9)
fs = pycbc.strain.strain.execute_cached_fft(ts)
print(ts._epoch, fs._epoch)

which prints:

1000000000 0
mj-will commented 1 month ago

@spxiwh I opted to make the change in a try/except block so that the function still works with Array inputs. I'm happy to change this if you have other suggestions

titodalcanton commented 1 month ago

Could you elaborate on what bug this fixes? Which scientific output does the bug affect (searches, PE…)?

mj-will commented 1 month ago

Could you elaborate on what bug this fixes? Which scientific output does the bug affect (searches, PE…)?

@titodalcanton scientific output might be a bit of a stretch but it was the best fit of the options listed.

The bug this fixes occurs if you call executed_cached_fft on an instance of TimeSeries (or on a FrequencSeries if using the inverse option). The output object does not have the _epoch attribute set, so it defaults to zero. I've updated the PR text with a minimal example that shows this.

This cropped up in some LISA-related short author work Ian, Gareth and I are working. We're using this function for performing faster FFTs/iFFTs with large time series/frequency series for both search and inference. The bug described here was leading to inconsistent epochs between the data and generated waveforms, which in turn led to incorrect results in the inference.