Scale Tukey alpha for long-duration signals#127
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds duration-based Tukey window roll-off configuration, preserves the prior four-second behaviour, validates parameter handling and FFT cache invalidation, documents the API, and cross-validates generated windows against bilby. ChangesTime-domain Tukey windowing
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/cross_validation/test_tukey_window.py (2)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations to the test function.
Adding type hints for the parameter and return type improves static analysis and maintainability.
💡 Proposed refactor
`@pytest.mark.parametrize`("duration", [4, 128]) -def test_default_tukey_window_matches_bilby(duration): +def test_default_tukey_window_matches_bilby(duration: int) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cross_validation/test_tukey_window.py` around lines 23 - 24, Add a type annotation for the duration parameter and annotate test_default_tukey_window_matches_bilby with its return type, preserving the existing parametrization and test behavior.
7-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify conditional dependency skipping with Pytest.
The manual
try/exceptblock andpytestmarkcan be replaced withpytest.importorskip(), which is Pytest's idiomatic way to handle missing test dependencies. It natively skips the module and can also retain the helpful installation instruction that was previously being swallowed by theexceptblock.♻️ Proposed refactor
-from tests.utils import check_bilby_available - -try: - check_bilby_available() - import bilby - - BILBY_AVAILABLE = True -except ImportError: - BILBY_AVAILABLE = False - -pytestmark = pytest.mark.skipif( - not BILBY_AVAILABLE, - reason="bilby required for cross-validation tests", -) +pytest.importorskip( + "bilby", + reason="cross_validation tests require bilby. Install with: pip install bilby", +) +import bilby(Note: Explicit
import bilbyis preserved after the skip check to ensure static type checkers like Pyright can still correctly resolve the module).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cross_validation/test_tukey_window.py` around lines 7 - 20, Replace the manual bilby availability check and module-level pytestmark with pytest.importorskip(), preserving the check_bilby_available() installation guidance before the skip and retaining the explicit bilby import afterward for static type checking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/cross_validation/test_tukey_window.py`:
- Around line 23-24: Add a type annotation for the duration parameter and
annotate test_default_tukey_window_matches_bilby with its return type,
preserving the existing parametrization and test behavior.
- Around line 7-20: Replace the manual bilby availability check and module-level
pytestmark with pytest.importorskip(), preserving the check_bilby_available()
installation guidance before the skip and retaining the explicit bilby import
afterward for static type checking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 159b9711-49d1-47ef-8808-1f1c80b9f034
📒 Files selected for processing (4)
docs/guides/data.mdsrc/jimgw/core/single_event/data.pytests/cross_validation/test_tukey_window.pytests/unit/core/single_event/test_data.py
| if roll_off < 0: | ||
| raise ValueError("roll_off must be non-negative") | ||
| duration = float(self.duration) | ||
| resolved_alpha = 0.0 if duration == 0 else 2 * roll_off / duration |
There was a problem hiding this comment.
This check for duration == 0 seems a bit extra; I would suggest removing it.
Such data would have thrown all sorts of errors early on.
There was a problem hiding this comment.
This guard is kept because Data() is used as an empty placeholder during detector construction and reset. Data.__init__() calls set_tukey_window(), so removing the guard would divide by zero before strain data is assigned. For the empty placeholder, alpha=0 produces an empty window.
| if roll_off < 0: | ||
| raise ValueError("roll_off must be non-negative") |
There was a problem hiding this comment.
Technically, a negative alpha will not lead to any errors and will return an identity window. Also, if we are checking roll_off, shouldn't we be checking alpha as well?
208dce5 to
233bd83
Compare
|
@SSL32081 I have added some more changes. |
3 participants
Summary
alphafrom a fixed 0.4-second roll-off using Bilby'salpha = 2 * roll_off / durationconventionalpha=0.2behavior for four-second segments while reducing a 128-second BNS segment toalpha=0.00625alphacontrol, add customroll_offcontrol and validation, and document both parameterizationsMotivation
Jim previously used a fixed
alpha=0.2, so the taper duration grew with the data segment. On a 128-second BNS segment this tapers 12.8 seconds on each side and can remove signal-rich late-inspiral data. A fixed roll-off time keeps the taper duration stable across BBH and long-duration BNS analyses.The 0.4-second default is compatibility-preserving for Jim's standard four-second analyses while adopting Bilby's duration-scaled calculation.
Validation
220 passed, 1 deselected— complete non-slow single-event unit suite plus Tukey cross-validation55 passed— focused data unit tests and Bilby cross-validation after the final type correctionjim-devrun retains the pre-existing BlackJAXlive_covarianceenvironment errorCloses #15
Partially addresses #23
Summary by CodeRabbit