Change timeseries, frequencyseries epoch to be float64 rather than ligotimegps #5237
Conversation
…ere (origins buffer, trace helpers, test instrumentation)
|
This has loads of stuff that Im doing for debugging, that will be removed when I have worked out what is up, so isn't ready for review yet |
There was a problem hiding this comment.
Pull request overview
This PR changes the epoch attribute of TimeSeries and FrequencySeries from lal.LIGOTimeGPS to float64 to reduce dependency on LAL. This is part of a broader effort (#5217) to make PyCBC less dependent on LAL libraries. The changes include:
- Replacing
LIGOTimeGPSwithfloat64for epoch storage in core types - Making LAL imports optional throughout the codebase using
import_optional - Replacing LAL constants with
pycbc.constantsmodule constants - Reimplementing some LAL-dependent functionality (XML PSD parsing, Gaussian noise generation)
Key Changes
- Core type system updated to use float64 epochs instead of LIGOTimeGPS
- LAL imports made optional via
import_optionalwrapper - Constants migrated from
lal.*topycbc.constants.*
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| test/validation_code/old_stat.py | Added debug_trace_candidate method for debugging |
| test/validation_code/old_coinc.py | Fixed syntax error in condition |
| test/test_timeseries.py | Removed lal imports, updated epoch to float64 |
| test/test_noise.py | Added Gaussian noise test class, refactored tests |
| test/test_live_coinc_compare.py | Added extensive debugging infrastructure |
| test/test_injection.py | Replaced lal constants/functions with pycbc equivalents |
| test/test_frequencyseries.py | Removed lal imports, updated epoch to float64 |
| test/test_frame.py | Replaced LIGOTimeGPS with float64 |
| test/test_constants.py | New test for pycbc.constants module |
| test/lalsim.py | Replaced lal.PI with pycbc PI constant |
| test/fft_base.py | Reorganized imports, removed lal dependency |
| pycbc/types/timeseries.py | Changed epoch from LIGOTimeGPS to float64 |
| pycbc/types/frequencyseries.py | Changed epoch from LIGOTimeGPS to float64 |
| pycbc/types/array.py | Made lal import optional |
| pycbc/waveform/*.py | Replaced lal constants with pycbc.constants |
| pycbc/psd/read.py | Reimplemented XML PSD parsing without lal.series |
| pycbc/noise/gaussian.py | Reimplemented noise generation without lalsimulation |
| pycbc/events/stat.py | Fixed sigmasq sqrt bug, added debug infrastructure |
| pycbc/events/coinc.py | Fixed syntax error, added origins tracking |
| pycbc/inject/inject.py | Replaced lal constants |
| pycbc/filter/matchedfilter.py | Changed epoch default from None to "" |
| pycbc/vetoes/chisq_cupy.py | Used pycbc.constants instead of lal |
| pycbc/tmpltbank/*.py | Replaced lal constants/imports |
| pycbc/results/versioning.py | Made lal/lalframe/lalsimulation imports optional |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/test_live_coinc_compare.py
Outdated
| # (though we only do a few iterations here) | ||
|
|
||
| def assess_same_output(newout, oldout): | ||
| def assess_same_output(idx, newout, oldout, new_coincer, old_coincer, trigs): |
There was a problem hiding this comment.
The debug helper function has grown to over 350 lines (lines 128-484), making it difficult to maintain and understand. Consider extracting sub-functions for different debug operations (e.g., debug_single_stat_comparison, debug_template_buffers, debug_replay_mapping) to improve readability and maintainability.
| import pycbc.version | ||
| from pycbc.libutils import import_optional | ||
|
|
||
| lal = import_optional('lal') |
There was a problem hiding this comment.
These changes maybe don't belong in here, but its as good a place as any I think
| Array of positions in the second dimension | ||
| """ | ||
| # Lalpulsar not a requirement for the rest of pycbc, so check if we have it | ||
| # Lal/Lalpulsar are not a requirement for the rest of pycbc, so check if we have it |
There was a problem hiding this comment.
Again not really part of the epoch changes, but just bundled in
| from pycbc.opt import LimitedSizeDict | ||
| from pycbc.libutils import import_optional | ||
|
|
||
| _lal = import_optional('lal') |
There was a problem hiding this comment.
This is only used in the lal() method, so its fairly safe to assume that someone using that would want lal installed!
| except AttributeError: | ||
| # nothing given, and can't grab the epoch from initial | ||
| # array - fall back to zero | ||
| epoch = float64(0) |
There was a problem hiding this comment.
I'm not 100% sure what the default should be. Was None intended as the "epoch doesn't matter" signifier before? Maybe we should fall back to that
There was a problem hiding this comment.
This is also repeated pretty much verbatim for both frequencyseries and timeseries, maybe there should be a function in array.py or similar?
| self.out_c2c_rev = [2.0+2.0j,4.0-4.0j] | ||
| # For Time/FrequencySeries, we want to test with a non-trivial epoch | ||
| self.epoch = LTG(3,4) | ||
| self.epoch = 3 + 4 * 1e-9 |
There was a problem hiding this comment.
This is the only change in the file that is actually related to the epoch change - the rest is general cleanup
|
This PR keeps failing the test Across most output datasets, outputs are identical, but one mismatch remains in the combined I tried to get copilot to help find out why, but it got fixated on some numpy-2 inconsistencies Again, the mismatch is very rare and small. But nonetheless, this is a correctness/regression issue worth noting because it indicates a change in numerical behavior. |
In the series of removing lal from pycbc, here I have made it so that the epoch of the array, timeseries, and frequencyseries types use float64 rather than ligotimegps
Standard information about the request
This is a: change to a core data type
This change affects: the offline search, the live search, inference, PyGRB
This change changes: hopefully nothing
This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
Motivation
Reduce dependency on LAL
Contents
Change the epoch data type to float64, allowing LIGOTimeGPS to be used to set this if wanted
Links to any issues or associated PRs
Split off from #5217
Testing performed
CI tests should work to test this, I will mark as draft so that I can make sure they all pass