FEAT: add option to crop whitened time series by ColmTalbot · Pull Request #1030 · bilby-dev/bilby

ColmTalbot · 2025-12-23T17:04:34Z

This PR implements the proposed modification to the likelihood from https://iopscience.iop.org/article/10.1088/1361-6382/ae1ac7/meta.

The main new feature is adding a crop_duration to the InterferometerStrainData and some accompanying utility code.

I modified how the GravitationalWaveTransient and BasicGravitationalWaveTransient call the data. I did not have to change the reference likelihood tests, so there is good evidence that this doesn't impact the numerical values when not using the cropping.

I suspect there is a performance penalty due to repeatedly whitening the data. There should be a way to cache this calculation, it just requires some cleverness about when the time/frequency masks might have changed.

I'm not sure at this time how this should interact with accelerated likelihood, so I've left them alone.
~~I also haven't validated how this interacts with time/calibration marginalization.~~

mj-will · 2026-01-22T15:46:28Z

+    @property
+    def frequency_mask(self):
+        frequencies = bilby.core.utils.series.create_frequency_series(
+            duration=self.ifo.cropped_duration, sampling_frequency=self.sampling_frequency
+        )
+        return (self.ifo.minimum_frequency <= frequencies) & (
+            frequencies <= self.ifo.maximum_frequency
+        )
+


We should probably have a utility function that does this.

GregoryAshton

Overall looks good and I'm supportive, just a few things I wanted to check.

asb5468 · 2026-02-05T15:50:32Z

+        else:
+            return self.whiten_and_crop(frequency_series=frequency_series)
+
+    def whiten_and_crop(self, frequency_series : np.array) -> np.array:


It seems like this procedure is more fundamental than suggested by its being a method of the interferometer class. The nice thing about using the gwutils SNR functions was that you could calculate SNRs using them without necessarily initializing an interferometer object. Is it possible to pull this out to the utils and just call it here?

GregoryAshton · 2026-02-09T10:05:53Z

+    return np.nan_to_num(whitened) * frequency_mask * (4 / duration)**0.5
+
+
+def whiten_and_crop(frequency_series, amplitude_spectral_density, frequency_mask, time_mask, duration):


Should the name state what input it accepts? E.g. like 'frequency_domain_whiten' to make it clear it is acting on the frequency-domain input?

I can make this change, but it's pretty clear from the signature and docstring.

GregoryAshton

A few more comments after another look.

GregoryAshton · 2026-02-09T10:07:59Z

-            * np.sqrt(np.sum(self.frequency_mask)) / frequency_window_factor
+            * np.sqrt(np.sum(self.frequency_mask))
+            / frequency_window_factor
+            * self.time_mask.mean()**0.5


Why does this use both np.sqrt and **0.5. It would probably be easier to read if it was consistent.

I think I was trying to avoid explicitly using numpy, but that's unavoidable since we need an fft and sqrt is sometimes meaningfully faster.

Suggested change

* self.time_mask.mean()**0.5

* np.sqrt(self.time_mask.mean())

GregoryAshton · 2026-02-09T10:09:58Z

            The frequency series, whitened by the ASD
        """
-        return frequency_series / (self.amplitude_spectral_density_array * np.sqrt(self.duration / 4))
+        if self.crop_duration == 0:


What happens (or can) crop_duration be "None" or None etc? This would silently fail and then trigger the else clause, likely also erroring, but could be confusing?

This would fail in the crop_duration.setter in the InterferometerStrainData with a reasonably explicit error message.

GregoryAshton · 2026-02-09T10:11:56Z

        )
-        _mask = interferometer.frequency_mask

        if 'recalib_index' in parameters:


Why does the signal no longer require masking here?

Unclear, is this tested?

Co-authored-by: Colm Talbot <talbotcolm@gmail.com>

Refactor likelihood tests to store results in variables before assertions.

asb5468

A few more general questions/comments:

In https://arxiv.org/pdf/2508.11091, there are two options presented for how to treat the template, "Signal models can then either be generated in the frequency domain with frequency spacing 1/T and whitened before subtracting from the whitened and cropped frequency-domain data or generated with the full duration T + 2∆ and processed using the same procedure as the data." Do I understand correctly that this MR has implemented the first option?
The code itself seems fine, but it's hard to tell definitively whether there are any unintended consequences, probably because I'm not familiar with all the review tests, and right now they are failing. Have you done any more extensive test runs that you could share, like a GW150914 analysis with the new likelihood with no cropping to make sure the results are unchanged?

asb5468 · 2026-07-02T15:02:03Z

-            duration=self.strain_data.duration)
+        whitened_signal = self.whiten_frequency_series(signal)
+        whitened_data = self.whitened_frequency_domain_strain
+        return (whitened_signal.T * whitened_data.conj()).sum()


Why does the whitened signal need to be transposed? In the function below, template_template_inner_product, it is not.

I suspect this is a typo, it likely wouldn't do anything for one dimensional arrays which I think we have an implicit expectation this is, but I'll remove it.

asb5468 · 2026-07-02T15:05:33Z

-            * np.sqrt(np.sum(self.frequency_mask)) / frequency_window_factor
+            * np.sqrt(np.sum(self.frequency_mask))
+            / frequency_window_factor
+            * self.time_mask.mean()**0.5


Where does this extra factor of self.time_mask.mean()**0.5 come from? Can you point to a specific equation in https://arxiv.org/pdf/2508.11091?

Note for myself later, it is (10), this is $\sqrt{\beta}$.

asb5468 · 2026-07-02T15:08:17Z

            A list of notches
+        crop_duration: float | tuple
+            The duration of data to crop at the beginning/end of the segment
+            to avoid whitening artifacts. If a float, that duration is excluded


The docstring here should add the units for this argument. I'm assuming it's seconds?

asb5468 · 2026-07-02T15:08:45Z

+    @property
+    def crop_duration(self):
+        """
+        The duration of data to crop at the beginning/end of the segment


Add the units of this property to the docstring. Also, crop_duration and cropped_duration are really similar and I'm leery of them getting mixed up. Perhaps crop_duration could be renamed to crop_time?

asb5468 · 2026-07-02T15:17:51Z

+            cropped_frequencies
+        )
+
+        self._frequency_mask_updated = True


It seems like self._frequency_mask_updated now controls whether both frequency_mask and cropped_frequency_mask are updated. Is there ever a situation where a user would want to update one but not the other?

I don't think so, I feel like we should require that they are kept synchronized.

asb5468 · 2026-07-02T15:20:51Z

-            signal[_mask] *= self.calibration_draws[interferometer.name][int(parameters['recalib_index'])]
+            signal *= self.calibration_draws[interferometer.name][int(parameters['recalib_index'])]


Why is this no longer being applied only to the masked frequencies?

asb5468 · 2026-07-02T15:24:15Z

-            d_inner_h_array = 4 / self.waveform_generator.duration * np.fft.fft(
-                d_inner_h_integrand[0:-1], axis=0
-            ).T
+            d_inner_h_array = np.fft.fft(d_inner_h_integrand[:-1], axis=0).T


Why is the 4/T normalization no longer applied here?

Ah, is it because it's already applied within interferometer.whitened_frequency_domain_strain and whitened_signal?

asb5468 · 2026-07-02T15:36:34Z

        with self.assertRaises(ValueError):
            self.ifo.inject_signal(injection_polarizations=None, parameters=None)

-    def test_optimal_snr_squared(self):


Why is this test removed?

ColmTalbot added enhancement New feature or request >100 lines detector likelihood to discuss To be discussed on an upcoming call labels Jan 20, 2026

ColmTalbot force-pushed the truncated-likelihood branch from f39871f to 5fbc454 Compare January 21, 2026 15:17

ColmTalbot requested review from a team, GregoryAshton, adivijaykumar, asb5468 and mj-will January 21, 2026 17:52

mj-will reviewed Jan 22, 2026

View reviewed changes

ColmTalbot force-pushed the truncated-likelihood branch from 0b74e2b to 0972d62 Compare January 23, 2026 16:53

ColmTalbot requested a review from mj-will January 26, 2026 08:32

GregoryAshton reviewed Jan 28, 2026

View reviewed changes

Comment thread bilby/gw/detector/interferometer.py Outdated

Comment thread bilby/gw/detector/interferometer.py

Comment thread bilby/gw/likelihood/base.py

Comment thread bilby/gw/likelihood/base.py

ColmTalbot commented Jan 28, 2026

View reviewed changes

Comment thread bilby/gw/likelihood/base.py

Comment thread bilby/gw/likelihood/base.py

Comment thread bilby/gw/detector/interferometer.py Outdated

Comment thread bilby/gw/detector/interferometer.py Outdated

ColmTalbot added this to the 3.0.0 milestone Jan 29, 2026

ColmTalbot force-pushed the truncated-likelihood branch 2 times, most recently from b8f4b97 to c7e762a Compare February 5, 2026 14:38

asb5468 reviewed Feb 5, 2026

View reviewed changes

GregoryAshton reviewed Feb 9, 2026

View reviewed changes

ColmTalbot added 8 commits February 18, 2026 15:45

FEAT: add option to crop whitened time series

5014f4d

FMT: fix precommits

c8453d0

Expose whiten and crop method

d102673

BUG: squash some bugs

e6c6e3c

TST: remove now obsolete test

45fc893

FEAT: add whitened frequeny mask

0e18517

TYPO: fix imports

bd9e29e

BUG: fix imports

7d6a399

GregoryAshton and others added 7 commits February 18, 2026 15:45

Update bilby/gw/detector/interferometer.py

06adb1e

Co-authored-by: Colm Talbot <talbotcolm@gmail.com>

DOC: fix docstring

1e7086e

REFACTOR: move whitening functions into gwutils

730a12e

BUG: pre-commit fixes

032763b

BUG: typo fix

f3bec33

TYPO: remove old argument

a52830d

TYPO: typo fixes in whitening function

85294b4

ColmTalbot force-pushed the truncated-likelihood branch from 962bc37 to 85294b4 Compare February 18, 2026 20:45

ColmTalbot added 8 commits May 15, 2026 10:46

Merge branch 'main' into truncated-likelihood

6438790

TEST: add back in removed test

f885be2

Merge branch 'main' into truncated-likelihood

3e9cfdd

TYPO: add missing close

fa77fe4

TST: fix likelihood tests

f58e8e2

Refactor likelihood tests to store results in variables before assertions.

BUG: fix array conjugation

b9d4e63

BUG: remove excess blank line [skip ci]

6d3fe39

TST: fix typos

5a92dbe

asb5468 reviewed Jul 2, 2026

View reviewed changes

		return np.nan_to_num(whitened) * frequency_mask * (4 / duration)**0.5


		def whiten_and_crop(frequency_series, amplitude_spectral_density, frequency_mask, time_mask, duration):

	* self.time_mask.mean()**0.5
	* np.sqrt(self.time_mask.mean())

		signal[_mask] *= self.calibration_draws[interferometer.name][int(parameters['recalib_index'])]
		signal *= self.calibration_draws[interferometer.name][int(parameters['recalib_index'])]

Uh oh!

Conversation

ColmTalbot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregoryAshton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregoryAshton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asb5468 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ColmTalbot commented Dec 23, 2025 •

edited

Loading