TST: change all likelihood and waveform tests to use more recent waveforms (#791) by akjm99 · Pull Request #897 · bilby-dev/bilby

akjm99 · 2025-01-29T13:27:28Z

No description provided.

…forms

GregoryAshton

Overall looks good thanks @akjm99. Just need to revert the cases where it is using an ROQ.

GregoryAshton · 2025-02-04T10:23:35Z


        trial_roq_paths = [
            "/roq_basis",
-            os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),


Here, I don't think we have ROQ data for XPHM yet, so this will need to be reverted (and may be why the CI is failing).

@mj-will I assume #897 was a "move towards" modern waveforms and it is okay to use the older waveforms for the ROQs to avoid having to figure out how to package them?

Yes, I think that's fine.

I'll note there is definitely ROQ data for XPHM on CIT but I don't think it's public (and is also huge).

GregoryAshton · 2025-02-04T10:23:54Z

        # Possible locations for the ROQ: in the docker image, local, or on CIT
        trial_roq_paths = [
            "/roq_basis",
-            os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),


Same as the case above

And applied to the comments as well

GregoryAshton · 2025-02-04T10:24:27Z

        # Possible locations for the ROQ: in the docker image, local, or on CIT
        trial_roq_paths = [
            "/roq_basis",
-            os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),


GregoryAshton · 2025-02-04T10:26:31Z

        # Possible locations for the ROQ: in the docker image, local, or on CIT
        trial_roq_paths = [
            "/roq_basis",
-            os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),


…forms

…nto update-waveforms-in-tests

mj-will · 2025-02-04T13:40:59Z

@akjm99 it looks like a commit from main has made it into the branch and is showing in the diff. I don't think it's from a merge or rebase so I'm slightly unsure what's happened to the history. @GregoryAshton any ideas?

akjm99 · 2025-02-04T13:54:03Z

Hi @mj-will. I am not sure how that happened sorry! I followed the contributing guidelines which said to pull upstream, make edits, and then push to fork (?). When I tried to push my changes to my fork, it said there were conflicts and that I had to merge my new changes and merge/rebase my branch to my main (?) (which I think I managed to do, at least I was able to push to my fork after that...)

mj-will · 2025-02-04T14:05:17Z

Hi @mj-will. I am not sure how that happened... I followed the contributing guidelines which said to pull upstream. When I tried to push my changes to my fork, it then said there were conflicts and that I had to merge my new changes and merge/rebase my branch to main (?) (which I think I managed to do, at least I was able to push to my fork after that...)

Hmm, I think the upstream changed between you initially making the branch and now (the commit is from another recent PR). I think you've followed the contributing guidelines correctly but we're missing a section on updating a PR with changes from main after it has been made. I've opened an issue to track this: #901.

Given we'll squash, this probably doesn't matter in this case, and it's probably more work to fix than it's worth.

mj-will · 2025-12-11T17:32:14Z

@akjm99 do you have time to have another look at this? I think ROQ tests just need switch to IMRPhenomPv2.

GregoryAshton · 2025-12-16T11:56:29Z

            frequency_nodes_linear=fnodes_linear,
            frequency_nodes_quadratic=fnodes_quadratic,
            reference_frequency=50.0,
-            waveform_approximant="IMRPhenomPv2",


I think this line should be using IMRPhenomPv2 since we are passing in the ROQ terms just above

mj-will · 2026-08-26T08:38:53Z

Hi @akjm99 do you have capacity to have another look at this? Unfortunately, I can't see what the test failures are but hopefully it's an easy fix.

akjm99 · 2026-09-10T09:39:04Z

Hi @mj-will, apologies; I thought I had managed to fix this already! I think you should be able to see the tests again now. Some of the unit tests are failing, but I am unsure why. I have a lot of work at the moment, but I will have another go at trying to debug this after I am back from the LVK.

Switching the test suite to IMRPhenomXPHM (PR bilby-dev#897) exposed two issues: - catch_waveform_errors only matched the exact IMRPhenomPv2 EDOM string, so out-of-domain errors from XPHM (XLAL_EFUNC) and XAS/D (XLAL_EINVAL) leaked out instead of returning None. Add a shared _is_waveform_domain_error helper recognising all three LAL domain-error codes, applied at all three catch_waveform_errors sites. - test_lalsim_SimInspiralChooseFDWaveform passed raw SI numbers as masses (~35 kg). Pv2 tolerated this but XPHM aborts the process. Scale the masses by lal.MSUN_SI so the inputs are physical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

GregoryAshton · 2026-10-15T09:22:22Z

@akjm99 and @mj-will, I got my French friend to look at this, and it turns out it is more interesting than just a CI failure. In short, it surfaced that we are not handling LALSimulation waveform errors well.

Root cause of the failing tests on #897

Switching the test suite from IMRPhenomPv2 to IMRPhenomXPHM/the X-family exposed two distinct problems, because XPHM reports bad inputs differently than Pv2 did.

1. `catch_waveform_errors` no longer caught domain errors (library bug)

TestRelbinBBH::test_waveform_error_catching_binned feeds a negative mass with catch_waveform_errors=True and expects the source model to return None. The error-swallowing logic in bilby/gw/source.py only matched the exact Pv2 error string:

EDOM = (e.args[0] == 'Internal function call failed: Input domain error')

Different approximants surface an out-of-domain point through different XLAL error codes:

Approximant	Exception message	LAL code	Caught before?
`IMRPhenomPv2` / `XP`	`Internal function call failed: Input domain error`	`XLAL_EDOM`	✅
`IMRPhenomXPHM` (freq. sequence)	`Internal function call failed`	`XLAL_EFUNC`	❌
`IMRPhenomXAS` / `D`	`Invalid argument`	`XLAL_EINVAL`	❌

So with XPHM the error propagated instead of being converted to a -inf likelihood, and the test failed.

Fix: a shared _is_waveform_domain_error() helper that recognises all three LAL domain-error messages, applied at all three catch_waveform_errors sites (standard FD path, frequency-sequence/relative-binning path, and the gwsignal path). This also closes a latent gap where XAS/D (Invalid argument) errors were never caught on the standard path.

2. Unit test fed unphysical masses (test bug)

test/gw/utils_test.py::test_lalsim_SimInspiralChooseFDWaveform passed raw SI numbers as the masses, so mass_1=35.2 meant 35.2 kg (~3×10⁻²⁹ M☉). IMRPhenomPv2 tolerated these nonsense values, but IMRPhenomXPHM aborts the process (SIGKILL) on them — which is why the run died mid-test with no traceback.

Fix: scale the masses by lal.MSUN_SI so the test passes physically valid inputs.

The XLAL_EFUNC message ("Internal function call failed") is fairly generic, so catch_waveform_errors=True now swallows a slightly broader class of nested LAL failures as -inf. This is consistent with the feature's intent (opt-in robustness during sampling),

The pre-commit pretty-format-toml hook (v2.14.0) now adds spaces inside inline-table braces. These files predated that style, causing the pre-commit CI job to fail. Purely cosmetic; no semantic changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

GregoryAshton · 2026-10-15T14:34:15Z

We now have one failure in the CI

FAILED test/gw/likelihood/roq_test.py::TestROQLikelihood::test_create_roq_weights_with_params - AssertionError: np.float64(407.93544108302456) != np.float64(407.93544108302467)
= 1 failed, 512 passed, 895 skipped, 59 warnings, 3 rerun in 576.63s (0:09:36) =

Tagging @ColmTalbot and @mj-will - can we just found these numbers as it looks like floating point precession rounding?

ColmTalbot · 2026-10-15T17:16:42Z

We now have one failure in the CI

FAILED test/gw/likelihood/roq_test.py::TestROQLikelihood::test_create_roq_weights_with_params - AssertionError: np.float64(407.93544108302456) != np.float64(407.93544108302467) = 1 failed, 512 passed, 895 skipped, 59 warnings, 3 rerun in 576.63s (0:09:36) =

Tagging @ColmTalbot and @mj-will - can we just found these numbers as it looks like floating point precession rounding?

Yeah, happy to make these almost equal at some sensible level.

ColmTalbot · 2026-10-15T17:20:02Z

I think we had deliberately not caught all of these previously, for example, is the error for the aligned waveforms due to passing precessing spins? I think we should not catch this error.

The two log-likelihood ratios can differ at the level of floating-point round-off, causing the exact equality check to fail on CI. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

GregoryAshton · 2026-10-16T07:57:54Z

I think we had deliberately not caught all of these previously, for example, is the error for the aligned waveforms due to passing precessing spins? I think we should not catch this error.

Good point @ColmTalbot I've now pushed a change which reverts the catching of "Invalid argument". There is a comment blocki above it which describes the behaviour - let me know if you prefer that to be removed. I then decided to add a test checking the behaviour across the different possible errors to check we are understand how the likelihood is behaving. I can update the PR accordingly if people are happy with this. Or I can remove it.

TST: change all likelihood and waveform tests to use more recent wave…

8634b6f

…forms

akjm99 linked an issue Jan 29, 2025 that may be closed by this pull request

Change all likelihood and waveform tests to use more recent waveforms #791

Open

Merge branch 'main' into update-waveforms-in-tests

ec6fed1

akjm99 requested a review from GregoryAshton January 29, 2025 16:28

GregoryAshton requested changes Feb 4, 2025

View reviewed changes

akjm99 added 4 commits February 4, 2025 05:22

TST: change all likelihood and waveform tests to use more recent wave…

ae08630

…forms

TST: update waveforms

bb82c36

Merge branch 'update-waveforms-in-tests' of github.com:akjm99/bilby i…

1794705

…nto update-waveforms-in-tests

TST: update waveforms

9451577

akjm99 requested a review from GregoryAshton February 4, 2025 13:41

mj-will added the testing label Feb 4, 2025

Merge branch 'main' into update-waveforms-in-tests

32ee9d0

BUG: update waveforms to IMRPhenomPv2 for ROQ tests

f57ad68

GregoryAshton reviewed Dec 16, 2025

View reviewed changes

akjm99 added 2 commits December 16, 2025 04:49

BUG: update waveforms to IMRPhenomPv2 for ROQ tests

94cc6dd

BUG: update waveforms to IMRPhenomPv2 for ROQ tests

3b82723

mj-will added the waiting for author input label Aug 26, 2026

Merge branch 'main' into update-waveforms-in-tests

a5aebd3

GregoryAshton and others added 2 commits October 14, 2026 20:29

Merge branch 'main' into update-waveforms-in-tests

0345e9a

GregoryAshton and others added 2 commits October 15, 2026 21:34

TST: use assertAlmostEqual for ROQ weights-with-params comparison

6bea049

The two log-likelihood ratios can differ at the level of floating-point round-off, causing the exact equality check to fail on CI. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

BUG: stop catching Invalid argument waveform errors and add tests

db2a332

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Conversation

akjm99 commented Jan 29, 2025

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!

Choose a reason for hiding this comment

Uh oh!

mj-will commented Feb 4, 2025

Uh oh!

akjm99 commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mj-will commented Feb 4, 2025

Uh oh!

mj-will commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mj-will commented Aug 26, 2026

Uh oh!

akjm99 commented Sep 10, 2026

Uh oh!

GregoryAshton commented Oct 15, 2026

Root cause of the failing tests on #897

1. catch_waveform_errors no longer caught domain errors (library bug)

2. Unit test fed unphysical masses (test bug)

Uh oh!

GregoryAshton commented Oct 15, 2026

Uh oh!

ColmTalbot commented Oct 15, 2026

Uh oh!

ColmTalbot commented Oct 15, 2026

Uh oh!

GregoryAshton commented Oct 16, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akjm99 commented Feb 4, 2025 •

edited

Loading

1. `catch_waveform_errors` no longer caught domain errors (library bug)