ENH: Improve strain data read in and downsampling - #926
Open
GregoryAshton wants to merge 6 commits into
Open
Conversation
Collaborator
|
@GregoryAshton is this still a draft or is it ready for review? |
mj-will
added
enhancement
GregoryAshton
changed the title
Collaborator
Author
|
@mj-will this is ready to review - I updated the title |
mj-will
requested changes
Aug 25, 2026
mj-will
left a comment
mj-will
left a comment
Collaborator
There was a problem hiding this comment.
Some initial comments @GregoryAshton.
I also think this would benefit from some tests.
AI summary: 1. resample_with_gwpy now accepts **kwargs passed through to .resample(), and fixed the bug where it discarded the result instead of returning it (it was returning None). 2. resample_with_lal accepts **kwargs for interface consistency, raising a clear ValueError if any are passed (LAL's resampler doesn't support extra options). 3. resample_timeseries passes **kwargs through to whichever resampling function is selected, and now returns data unchanged when no resampling is needed (previously returned None in that branch too). 4. find_and_read_data gained a resample_kwargs parameter so resampling kwargs can flow all the way through; fixed the docstring to clarify ifo isn't limited to H1/L1/V1 (K1, A1 work fine via ifo[0]); fixed the "not include" grammar; and fixed a latent bug where find_url_kwargs/read_kwargs defaulting to None would crash on **None. 5. Added a TestResampling and TestFindAndReadData test class covering the resampling functions and find_and_read_data (mocking gwdatafind.find_urls and TimeSeries.read), addressing his request for test coverage.
Collaborator
Author
|
@mj-will I pushed a change addressing your comments |
There was a problem hiding this comment.
Pull request overview
This PR enhances Bilby’s GW strain data utilities by adding (1) a gwdatafind-backed helper to locate and read frame data via GWPy, and (2) a unified downsampling/resampling interface that can use either GWPy or LAL, with accompanying unit tests.
Changes:
- Added
find_and_read_data()to locate frame URLs withgwdatafind.find_urls()and load them viagwpy.timeseries.TimeSeries.read(). - Added
resample_timeseries()plus backend helpersresample_with_gwpy()/resample_with_lal()to support configurable downsampling. - Added tests covering resampling behavior and the new data-finding/read helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
bilby/gw/detector/strain_data.py |
Adds new resampling helpers and a gwdatafind + GWPy data discovery/read function. |
test/gw/detector/strain_data_test.py |
Adds unit tests for the new resampling functions and find_and_read_data(). |
Suppressed comments (1)
bilby/gw/detector/strain_data.py:1006
- Other optional dependency entrypoints in this module wrap
gwpyimports to raise a clearerModuleNotFoundErrormessage.find_and_read_data()currently importsgwpy/gwdatafinddirectly, which will surface a less helpful error if those extras aren't installed.
from gwpy.timeseries import TimeSeries
from gwdatafind import find_urls
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1021
to
+1024
| type_kwargs = dict(dtype=dtype, subok=True, copy=False) | ||
| data = TimeSeries.read(urls, channel_with_ifo, start=start, end=end, **read_kwargs).astype(**type_kwargs) | ||
| data = resample_timeseries(data, sampling_frequency, resampling_method, **resample_kwargs) | ||
| return data |
Comment on lines
+915
to
+919
| if kwargs: | ||
| raise ValueError(f"resample_with_lal does not support additional kwargs: {kwargs}") | ||
| import lal | ||
| from gwpy.timeseries import TimeSeries | ||
| lal_timeseries = data.to_lal() |
Comment on lines
+531
to
+542
| @mock.patch("gwdatafind.find_urls") | ||
| @mock.patch("gwpy.timeseries.TimeSeries.read") | ||
| def test_find_and_read_data_resamples(self, mock_read, mock_find_urls): | ||
| mock_find_urls.return_value = ["file://fake/H-H1_HOFT_C00_AR-0-4.gwf"] | ||
| mock_read.return_value = self.data | ||
|
|
||
| data = find_and_read_data( | ||
| self.start, self.end, self.ifo, self.frametype, self.channel, | ||
| sampling_frequency=self.sampling_frequency / 2, | ||
| ) | ||
|
|
||
| self.assertEqual(data.sample_rate.value, self.sampling_frequency / 2) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
test/gw/detector/strain_data_test.py:490
- These tests rely on
gwdatafind(via the patch decorators) and GWPy. Without an explicit skip insetUp, they will error in environments wheregwdatafindisn’t installed. Add a guard so the test class is skipped unless both GWPy and gwdatafind are available.
def setUp(self):
self.start = 0
self.end = 4
self.ifo = "H1"
Comment on lines
4
to
+6
| import numpy as np | ||
| import scipy.signal | ||
| from gwpy.timeseries import TimeSeries |
Comment on lines
+431
to
+433
| def setUp(self): | ||
| self.sampling_frequency = 512 | ||
| self.data = TimeSeries( |
Comment on lines
+967
to
+972
| if data.sample_rate.value == sampling_frequency: | ||
| logger.info("Sample rate matches data no resampling") | ||
| return data.copy() | ||
| elif resampling_method in RESAMPLING_FUNCTIONS: | ||
| logger.info(f"Resampling data to sampling_frequency {sampling_frequency} using {resampling_method}") | ||
| return RESAMPLING_FUNCTIONS[resampling_method](data, sampling_frequency, **kwargs) |
Collaborator
|
Hmm, I had hoped asking copilot for a re-review would make it close the threads it opened but seems not :/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This MR:
find_and_read_datafunction to enable rapid finding of data usinggw_data_find.gwpyorlaldownsampling (ported from bilby_pipe)