Skip to content

Add unit testing#18

Open
prayush wants to merge 3 commits into
gwnrtools:masterfrom
prayush:add_unit_testing
Open

Add unit testing#18
prayush wants to merge 3 commits into
gwnrtools:masterfrom
prayush:add_unit_testing

Conversation

@prayush
Copy link
Copy Markdown
Contributor

@prayush prayush commented May 8, 2026

This PR adds substantial new unit test coverage for waveform generation, waveform utilities, blending helpers, and legacy fitting functions.

Generator and mode-generation tests

New mocked tests were added for inspiral and merger-ringdown mode generation using patched lalsimulation and delegated generators. These tests verify:

  • correct handling of requested (l,m) modes,
  • inclusion/exclusion of conjugate modes,
  • correct return types (pycbc.TimeSeries vs NumPy arrays),
  • proper distance conversion before C-layer calls,
  • avoidance of duplicate mode generation calls,
  • correct eccentricity index selection logic,
  • correct error propagation for missing approximants,
  • filtering of requested modes from linked-list structures,
  • correct routing between lalsimulation and pyseobnr backends,
  • default handling of f_ref and coa_phase.

Deterministic numerical tests

Additional pure-Python tests (without mocks) were added for utils.py, blend.py, and legacy.py.

These tests verify:

  • consistency and scaling relations for waveform frequency utilities,
  • expected ISCO frequency behavior with spin,
  • peak-finding correctness,
  • polarization reconstruction and symmetry properties,
  • amplitude, phase, and frequency extraction,
  • mismatch normalization and orthogonality limits,
  • smoothness and endpoint behavior of blended waveforms,
  • correctness of phase alignment operations,
  • stability and positivity of legacy fitting functions.

Overall, this PR improves regression protection and numerical validation across the waveform generation and hybridization codebase while keeping the tests deterministic and lightweight for CI.

@prayush prayush assigned prayush and unassigned prayush May 8, 2026
@prayush prayush added the enhancement New feature or request label May 8, 2026
@prayush prayush force-pushed the add_unit_testing branch from 9c32aa3 to 7aac119 Compare May 8, 2026 09:13
@prayush
Copy link
Copy Markdown
Contributor Author

prayush commented May 8, 2026

Note that all changes to files in esigmapy/ and esigmapy/surrogate/ are simply reformatting done with black. New code is only in tests/.

prayush added 3 commits May 14, 2026 17:52
  and mocked coverage for inspiral and merger-ringdown generators

- Add unit tests for inspiral and MR mode generation utilities using
mocked lalsimulation / delegated generators.
- Add deterministic unit tests for waveform utilities, blending helpers,
and legacy fitting functions without using mocks.

utils.py
* verify f22/x roundtrip consistency across x and mass ranges
* verify f22 scales as M^-1
* verify Schwarzschild ISCO frequency for zero spins
* verify prograde spins increase ISCO frequency
* verify retrograde spins decrease ISCO frequency
* verify monotonic inputs produce no detected peaks
* verify sinusoidal inputs return expected peak locations
* verify equatorial-plane polarization reconstruction for (2,2) mode
* verify face-on symmetry gives hx = 0
* verify polarization linearity under mode scaling

blend.py
* verify first-value lookup on constant arrays
* verify first/last index lookup on increasing arrays
* verify first/last symmetry properties
* verify amplitude extraction for known complex inputs
* verify amplitude for real-valued inputs
* verify phase unwrapping for linear complex phase
* verify constant-phase inputs produce flat phase
* verify frequency recovery from linear phase evolution
* verify decreasing phase produces negative frequencies
* verify mismatch is zero for identical waveforms
* verify mismatch is one for orthogonal waveforms
* verify mismatch remains within [0, 1]
* verify blended series matches endpoints correctly
* verify blended series smoothness across transition region
* verify blended series output length
* verify phase alignment reduces mismatch
* verify inverse alignments cancel net phase shift

legacy.py
* verify zero polynomial coefficients return unity
* verify quadratic polynomial evaluation for known coefficients
* verify equal numerator/denominator ratio polynomial returns unity
* verify fitting functions remain positive for η ∈ (0, 0.25]
* verify fitting functions are finite at η = 0.01 and η = 0.25

Included test coverage:

* TestGetInspiralEsigmaModesWithMock (7 tests)
  - verify returned dict contains requested (l,m) key
  - verify include_conjugate_modes=True/False controls (l,-m)
  - verify default return type is pycbc.TimeSeries
  - verify return_pycbc_timeseries=False returns numpy arrays
  - verify distance conversion from Mpc to SI before C call
  - verify mode generation called once per unique mode

* TestEccentricityAtReferenceFrequencyWithMock (2 tests)
  - verify nearest-index lookup logic for x_ref
  - verify returned eccentricity matches argmin|x - x_ref|

* TestGenerateLalsimModesWithMock (5 tests)
  - verify missing approximant propagates AttributeError -> IOError
  - verify linked-list traversal filters requested modes only
  - verify all requested modes are returned
  - verify unrequested modes are ignored
  - verify extracted mode data is returned as numpy arrays

* TestGetMrModesWithMock (4 tests)
  - verify f_ref=None defaults to f_lower
  - verify coa_phase=None defaults to 0.0
  - verify NRSur7dq4 routes to _generate_lalsim_modes
  - verify SEOBNRv5HM routes to _generate_pyseobnr_modes
@prayush prayush force-pushed the add_unit_testing branch from 246416a to aa4bc95 Compare May 14, 2026 12:24
Copy link
Copy Markdown
Collaborator

@Akash-Maurya-0899 Akash-Maurya-0899 left a comment

Choose a reason for hiding this comment

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

Hi @prayush, I have added some comments. The major one is regarding the black formatting test, which may simply fail due to different black versions used by the tests vs by a developer on their local machine. The fix would be to pin a particular version black for the tests, and also add it as an optional "developer" dependency of esigmapy. More details in the comments.

Comment thread tests/test_formatting.py
[
sys.executable,
"-m",
"black",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Pin a particular version of black. The black formatting tests failed for me as I had a different version of black installed on my local environment.

It would then also be good to add this same version of black as an optional dependency for the developers only, by adding these dependencies under a dev header in the setup.py file

extras_require={
    "surrogate": [
        "tpi-splines",
        "numba",
    ],
    "pyseobnr": ["pyseobnr"],
    "dev": [
        "black==xx.xx.x",  # pinned to avoid formatting disagreements
        "pytest",
    ],
    "all": [
        "tpi-splines",
        "numba",
        "pyseobnr",
    ],

},

Comment thread tests/test_blend.py
# 1.5 lies between arr[1]=1 and arr[2]=2; nearest is equidistant → either valid
arr = np.array([0.0, 1.0, 2.0, 3.0])
idx = find_first_value_location_in_series(arr, 1.5)
assert idx in (1, 2)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it check if the output is 1, and not 1 or 2 (because that's what find_first_value_location_in_series would output I think)? Or is it that due to floating point rounding error, it can be either 1 or 2?


- name: Install dependencies
run: |
pip install lalsuite pycbc pytest black
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread tests/test_blend.py

class TestFindFirstValue:
def test_below_min_raises(self):
with pytest.raises(Exception, match="[Ll]ower"):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure if this matching of "Lower/lower" in the exception message is needed; the TestFindLastValue set of tests below don't use it. If keeping it, please add a similar string matching requirement for the TestFindLastValue sets of tests below also.

Comment thread tests/test_blend.py
arr = np.linspace(1.0, 5.0, 50)
idx = find_last_value_location_in_series(arr, 3.0)
assert 0 <= idx < len(arr)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Counterparts of test_nearer_left_element_chosen, test_nearer_right_element_chosen, test_returns_first_bracket_index, test_exact_match_at_start of TestFindFirstValue suite of tests missing for TestFindLastValue suite of tests here.

Comment thread tests/test_blend.py
idx = find_first_value_location_in_series(arr, 0.7)
# First bracket is [0→1]; nearest of 0.0 and 1.0 to 0.7 is 1.0 → idx 1
assert idx <= 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

test_result_within_valid_range like test below is missing here.

Comment thread tests/test_blend.py
# |3 + 4j| == 5
wave = np.full(10, 3.0 + 4.0j)
amp = compute_amplitude(wave)
assert np.allclose(amp, 5.0)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Perhaps decrease the tolerances rtol and atol in np.allclose everywhere to make the tests more stringent.

Comment thread tests/test_blend.py
idx = self._all_idx(len(w1))
assert mismatch_discrete(w1, w2, idx, idx) >= 0

def test_subsample_indices(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure why this particular test is needed...


def test_case_sensitive(self):
with pytest.raises(IOError):
check_available_mr_approximants("nrsur7dq4")
Copy link
Copy Markdown
Collaborator

@Akash-Maurya-0899 Akash-Maurya-0899 May 14, 2026

Choose a reason for hiding this comment

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

Not a review of this PR, but looking at this, we should perhaps include support for case-insensitive MR approximant names in future.


def test_lalsim_contains_seobnrv4phm(self):
assert "SEOBNRv4PHM" in LALSIM_APPROXIMANTS

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add similar checks for the supported pyseobnr approximants as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants