Separate IMRPhenomHM implementation#102
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds IMRPhenomHM frequency‑domain higher‑mode waveform: new generator and mode‑projection utilities, interface class and preset registration, package export, test integrations, and benchmark/CLI script updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Gen as gen_IMRPhenomHM
participant LAL as XLALSimIMRPhenomHMGethlmModes
participant Project as get_phenomHMFD_mode_projection
participant Reducer as SumModes
Caller->>Gen: call(frequency_array, m1,m2,chi1,chi2,distance,inclination,phi0,f_ref)
Gen->>LAL: request mode amplitudes (ModeArray extra_params)
LAL-->>Gen: h_lm(f) for each mode
Gen->>Project: vectorised per‑mode projection (theta, minus1l, ell, m)
Project-->>Gen: complex projection factors per mode
Gen->>Reducer: multiply projections by h_lm and amp0, sum modes
Reducer-->>Gen: hp, hc
Gen-->>Caller: return (hp, hc)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new non-precessing higher-modes waveform implementation (IMRPhenomHM) intended to provide a faster alternative to IMRPhenomXPHM when precession is not needed, and wires it into the existing LAL cross-validation test suite.
Changes:
- Added
src/ripplegw/waveforms/IMRPhenomHM.pyimplementinggen_IMRPhenomHMby summing selected(ℓ,m)modes with spin-weighted spherical harmonic projections. - Extended the test utilities and LAL mismatch cross-validation to include
IMRPhenomHM. - Updated default frequency settings used by the LAL mismatch tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/ripplegw/waveforms/IMRPhenomHM.py |
New HM waveform generator built from XLALSimIMRPhenomHMGethlmModes + mode projection and summation. |
tests/utils.py |
Adds IMRPhenomHM support to JAX waveform generation and LAL waveform generation paths; adds import-time debug output. |
tests/cross_validation/test_lal_mismatch.py |
Includes IMRPhenomHM in parametrized mismatch tests; increases default f_u/f_sampling. |
src/ripplegw/__init__.py |
Adds IMRPhenomHM to __all__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@robkamcha do you also see the chi eff scaling for XPHM? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/ripplegw/waveforms/IMRPhenomHM.py (1)
41-45: Share the activated-mode list with the LAL harness.Line 41 hard-codes the same five
(ell, m)pairs thattests/utils.pyLines 348-351 repeat when building the LAL reference. Keeping two copies makes it easy for the implementation and oracle to drift apart, so please lift this into a shared constant and reuse it in both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ripplegw/waveforms/IMRPhenomHM.py` around lines 41 - 45, The list of activated modes is hard-coded in extra_params["ModeArray"] inside IMRPhenomHM.py and duplicated in tests/utils.py; extract this jnp.array of (ell,m) pairs into a single shared constant (e.g., MODES or MODE_ARRAY) in a common module (or package-level constants file) and import it from both IMRPhenomHM.py and tests/utils.py, then replace the inline array in extra_params (ModeArray) with the shared constant so both implementation and test harness use the same source of truth.tests/cross_validation/test_lal_mismatch.py (1)
321-322: Refresh the fixture docstring after doubling the sample rate.The note above still quotes
~32K/~257Kbins, but at 4096 Hz sampling those figures are roughly doubled. Keeping the sizing guidance accurate helps people choose sensible local and CI settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cross_validation/test_lal_mismatch.py` around lines 321 - 322, The fixture docstring describing bin counts was not updated after doubling f_sampling from ~2048 to 4096 Hz; find the fixture (mentions f_u and f_sampling) and update the approximate bin counts and sizing guidance accordingly (e.g., roughly double the previous "~32K" / "~257K" to about "~64K" / "~514K" or similar accurate values at 4096 Hz) so the note matches the new sampling rate and helps choose local/CI settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ripplegw/__init__.py`:
- Line 21: The name "IMRPhenomHM" is listed in __all__ but not imported, causing
exports like from ripplegw import IMRPhenomHM to fail; add an import for
IMRPhenomHM at the top-level of the package alongside the other preset imports
(the same place other preset modules are imported) so the symbol is bound before
it is exported in __all__ and matches the exact module/class name "IMRPhenomHM".
In `@tests/utils.py`:
- Line 26: Remove noisy prints during module import: in the import-except block
that sets LAL_AVAILABLE, stop calling sys.stdout.write and traceback.print_exc
unconditionally; either remove those calls or guard them behind an explicit
debug flag/environment variable (e.g., TESTS_DEBUG) and only emit via
logging.debug when enabled. Also remove or replace the unconditional print of
LAL_AVAILABLE at the bottom of the module with a debug log (logging.debug) or
drop it entirely so importing tests no longer emits output; changes should
target the import-except block that defines LAL_AVAILABLE and the site that
prints LAL_AVAILABLE.
---
Nitpick comments:
In `@src/ripplegw/waveforms/IMRPhenomHM.py`:
- Around line 41-45: The list of activated modes is hard-coded in
extra_params["ModeArray"] inside IMRPhenomHM.py and duplicated in
tests/utils.py; extract this jnp.array of (ell,m) pairs into a single shared
constant (e.g., MODES or MODE_ARRAY) in a common module (or package-level
constants file) and import it from both IMRPhenomHM.py and tests/utils.py, then
replace the inline array in extra_params (ModeArray) with the shared constant so
both implementation and test harness use the same source of truth.
In `@tests/cross_validation/test_lal_mismatch.py`:
- Around line 321-322: The fixture docstring describing bin counts was not
updated after doubling f_sampling from ~2048 to 4096 Hz; find the fixture
(mentions f_u and f_sampling) and update the approximate bin counts and sizing
guidance accordingly (e.g., roughly double the previous "~32K" / "~257K" to
about "~64K" / "~514K" or similar accurate values at 4096 Hz) so the note
matches the new sampling rate and helps choose local/CI settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62ae0599-e7bd-4c93-9d40-53015215c7b9
📒 Files selected for processing (4)
src/ripplegw/__init__.pysrc/ripplegw/waveforms/IMRPhenomHM.pytests/cross_validation/test_lal_mismatch.pytests/utils.py
@ThibeauWouters yes, just ran it for 256 samples with |
|
I'll open a new issue and check if |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/utils.py`:
- Around line 203-223: Update the misleading inline comment inside the waveform
wrapper for IMRPhenomHM: replace the phrase "consistent with the
precessing-waveform convention used by this test suite" with a brief note that
IMRPhenomHM is a non-precessing higher-modes waveform and that theta = [Mc, eta,
s1z, s2z, dist_mpc, tc, phic, inclination] is used for parameter ordering with
Mc_eta_to_ms handling mass conversion; ensure this change is made in the
waveform function that calls gen_IMRPhenomHM and uses Mc_eta_to_ms and theta.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43d4d515-00ad-4847-9c97-4679077a285b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/ripplegw/__init__.pysrc/ripplegw/interfaces.pytests/utils.py
✅ Files skipped from review due to trivial changes (1)
- src/ripplegw/init.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ripplegw/benchmarks/timings/timing.py (1)
382-391: Centralise waveform allow-lists to avoid drift across tooling.Line 390 is correct, but the same model registry is hard-coded in multiple places (
src/ripplegw/benchmarks/timings/timing.py,timings/submit_slurm.sh,timings/submit_condor.sh). Consider a shared constant (or generated list) so CLI choices and scheduler matrices cannot diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ripplegw/benchmarks/timings/timing.py` around lines 382 - 391, The hard-coded waveform model list used as the argparse "choices" (the list containing "IMRPhenomXPHM", "IMRPhenomXAS", "IMRPhenomD", etc.) must be centralised: extract that list into a single module-level constant (e.g., WAVEFORM_MODELS) in a shared Python module and replace the inline list in timing.py (the argparse choices) with a reference/import to WAVEFORM_MODELS; then make the scheduler scripts (submit_slurm.sh and submit_condor.sh) consume the same canonical list (either by calling a small helper like get_waveform_models() that prints the list or by importing the module in any Python-based job-matrix generator) so CLI choices and scheduler matrices derive from the same source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ripplegw/benchmarks/timings/timing.py`:
- Around line 382-391: The hard-coded waveform model list used as the argparse
"choices" (the list containing "IMRPhenomXPHM", "IMRPhenomXAS", "IMRPhenomD",
etc.) must be centralised: extract that list into a single module-level constant
(e.g., WAVEFORM_MODELS) in a shared Python module and replace the inline list in
timing.py (the argparse choices) with a reference/import to WAVEFORM_MODELS;
then make the scheduler scripts (submit_slurm.sh and submit_condor.sh) consume
the same canonical list (either by calling a small helper like
get_waveform_models() that prints the list or by importing the module in any
Python-based job-matrix generator) so CLI choices and scheduler matrices derive
from the same source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d68ed239-0e7c-427e-9da6-1bb997ed4822
📒 Files selected for processing (5)
README.mddocs/index.mdsrc/ripplegw/benchmarks/timings/timing.pytimings/submit_condor.shtimings/submit_slurm.sh
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/index.md
This PR adds
IMRPhenomHM. It's essentiallyIMRPhenomXPHMwithout the precession code, so it should run faster if you want higher modes in a non-precessing system.Here is the standard testing script's output for
T=16sandf_sampling=4096HzThere is still some worrying scaling with$\chi_{eff}$ that I'm looking into
Summary by CodeRabbit
New Features
Tests
Documentation
Chores