feat: temporal apodization windows for PhasorDetector - #428
Open
bruxillensis wants to merge 4 commits into
Open
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:
📝 WalkthroughWalkthroughAdds reusable Gaussian, linear-ramp, and Tukey envelopes with public window profiles, integrates optional temporal apodization into ChangesTemporal windows and apodization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant FDTDSimulation
participant PhasorDetector
participant WindowProfile
participant PhasorAccumulation
participant LeakageAssertions
FDTDSimulation->>PhasorDetector: update sampled fields
PhasorDetector->>WindowProfile: obtain cached window weight
WindowProfile-->>PhasorDetector: return window weight
PhasorDetector->>PhasorAccumulation: accumulate weighted phasor
PhasorAccumulation-->>PhasorDetector: apply coherent-gain scaling
PhasorDetector-->>LeakageAssertions: return detector magnitudes
LeakageAssertions->>LeakageAssertions: compare amplitude and leakage thresholds
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/fdtdx/core/window.py`:
- Around line 74-110: Validate constructor parameters in
GaussianWindowProfile.__post_init__ and TukeyWindowProfile.__post_init__: reject
sigma_time <= 0, end_time <= start_time, and alpha values outside the inclusive
[0, 1] range, using the project’s established validation/error convention. Add
regression tests covering each invalid case while preserving valid window
behavior.
In `@src/fdtdx/objects/detectors/phasor.py`:
- Around line 150-156: Update the class-level scaling documentation near
_static_scale to describe continuous-mode scaling as 2 / sum(w), including when
apodization is enabled, instead of stating 2 / N. Keep the existing pulse-mode
documentation unchanged.
- Around line 138-147: In the window-building logic that caches
_window_at_time_step_arr and _window_sum, validate immediately after masking
that the coherent-gain sum is finite and strictly positive, rejecting zero,
negative, or non-finite sums and weights before phasor processing can continue.
Add a regression test covering an out-of-interval/underflowed window and
non-finite weights, ensuring invalid windows are rejected.
🪄 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 Plus
Run ID: 29baa815-65d4-44e5-ad86-baeb775fc01d
📒 Files selected for processing (7)
docs/source/07_api.rstsrc/fdtdx/__init__.pysrc/fdtdx/core/window.pysrc/fdtdx/objects/detectors/phasor.pysrc/fdtdx/objects/sources/profile.pytests/unit/core/test_window.pytests/unit/objects/detectors/test_apodization.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
+ Coverage 90.52% 90.67% +0.14%
==========================================
Files 92 94 +2
Lines 11920 12229 +309
Branches 1820 1871 +51
==========================================
+ Hits 10791 11089 +298
- Misses 783 788 +5
- Partials 346 352 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bruxillensis
force-pushed
the
pr2-detector-apodization
branch
from
July 29, 2026 15:11
7ab7f8a to
d2defef
Compare
Adds an optional `apodization` window to `PhasorDetector`, applied to each sample of its running DFT. The default (`None`) is the existing hard rectangular gate, so nothing changes unless it is set. A rectangular gate leaks spectrally: a signal still ringing when the detector switches off hits a discontinuity, and the sidelobes land on neighbouring frequencies. Measuring a feature 1000x below a nearby strong tone, the gate overstates it by ~60x; a Tukey window brings it to the right order. Windows live in `core/window.py` alongside the envelope helpers, which are extracted from the inline copies in `objects/sources/profile.py` so the source profiles and the windows share one definition of each shape. `WindowProfile` takes `get_window(time)`, with `apodization` typed to it so a source profile is rejected at construction. Continuous-mode scaling becomes the window's coherent gain `2 / sum(w)`, which reduces to the previous `2 / N` for the rectangular case. The sum is taken over the recorded steps, so this composes with `dft_subsample`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bruxillensis
force-pushed
the
pr2-detector-apodization
branch
from
July 29, 2026 17:16
d2defef to
510c099
Compare
Runs a CW plane wave and reads the same plane with two PhasorDetectors, one rectangular and one Tukey, probing the tone plus two off-tone frequencies. Both detectors are gated to the steady-state part of the run. The source's linear startup ramp makes the field non-stationary for the first few periods, and a rectangular gate weights that transient equally while a taper de-weights it; ungated, that alone shifted the on-frequency amplitude by 6.7% for reasons unrelated to leakage. Gated, the two agree to 0.05%, so the coherent-gain correction is pinned to 1%. Probe frequencies sit at fractional bin offsets -- integer offsets land on the nulls of the rectangular window's kernel and hide the leakage entirely. Measured suppression is 3.0x at 2.5 bins and 3.9x at 4.5 bins; the test requires 2x, and separately asserts the rectangular leakage is present so the comparison cannot pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/simulation/physics/detectors/test_apodization_leakage.py (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the debug
The module-scoped fixture prints the spectra on every run, adding noise to CI output. If the values are useful for triage, emit them only on failure via the existing assertion messages.
♻️ Proposed cleanup
- print({k: np.array2string(v, precision=4) for k, v in out.items()}) return out🤖 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/simulation/physics/detectors/test_apodization_leakage.py` at line 113, Remove the unconditional debug print from the module-scoped fixture in test_apodization_leakage.py. Preserve any useful diagnostic spectra by including them in the existing assertion messages so they are emitted only when the assertions fail.
🤖 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/simulation/physics/detectors/test_apodization_leakage.py`:
- Line 113: Remove the unconditional debug print from the module-scoped fixture
in test_apodization_leakage.py. Preserve any useful diagnostic spectra by
including them in the existing assertion messages so they are emitted only when
the assertions fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca7660f8-89bf-4a76-aaee-1d5fad53fa2d
📒 Files selected for processing (1)
tests/simulation/physics/detectors/test_apodization_leakage.py
Leftover from working out the tolerances; the values it printed are recorded in the assertions and their docstrings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shipped assertions carried 12x-63,000x unused slack: they caught the gross defect they were written for but would not have caught drift. Tolerances are now set from measured values, per an audit that found no source defect. on-frequency amplitude (sim) rel=1e-2 -> 2e-3 (measured 8.5e-4) leakage suppression (sim) >2x -> >2.5x (measured 3.01x, 3.85x) CW amplitude, no window abs=2e-2 -> 2e-3 (measured 5.7e-4) CW amplitude, Tukey abs=3e-2 -> 1e-5 (measured 4.8e-7) CW amplitude, Tukey + stride abs=3e-2 -> 1e-5 (measured 2.4e-7) The apodized cases are ~1000x more accurate than the rectangular one because the taper suppresses the single-frequency DFT's 2w residual; 1e-5 is ~84 float32 ULP, so it is not tightened further. Two tolerance-free tests are added. A rectangular Tukey (alpha=0) must reproduce the un-apodized phasor bit-for-bit, which pins window construction and its alignment with the OnOffSwitch mask -- an off-by-one at either recording edge is absorbed by every amplitude tolerance above but fails this immediately. And suppression must grow with bin offset, which catches a window that is applied but wrong where a flat ratio bound would not. The subsample sweep moves from strides 1/4/8 to 1/2/3/4. Stride 8 leaves 2.2 samples per period and aliases by design -- the detector warns about it -- so its amplitude was necessarily bounded 30x looser while exercising no mechanism that stride 4 does not; test_phasor_subsample.py covers heavy strides. Verified: a mutation dropping the gain correction is caught by 6 tests, and a 0.4% scale drift that passed every old tolerance now fails 6. The simulation amplitude test still passes under drift by design -- it is a ratio of two detectors, so a common scale error cancels, which is what isolates the window's effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@ymahlau I think this is ready for a review. I tried to be more thorough in testing. |
Collaborator
|
Thank you for this PR and the thorough test cases. My only gripe is with the naming scheme. Having *Profile and *WindowProfile classes that do different things might be confusing to other users. What do you think about using TemporalWindow / GaussianWindow / TukeyWindow instead? Other than that, it looks fine to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an optional
apodizationwindow toPhasorDetector, applied to each sample of its running DFT. The default (None) is the existing hard rectangular gate, so nothing changes unless it is set.A rectangular gate leaks spectrally: a signal still ringing when the detector switches off hits a discontinuity, and the sidelobes land on neighbouring frequencies. Measuring a feature 1000x below a nearby strong tone, the gate overstates it by ~60x; a Tukey window brings it to the right order.
Windows live in
core/window.pyalongside the envelope helpers, which are extracted from the inline copies inobjects/sources/profile.pyso the source profiles and the windows share one definition of each shape.WindowProfiletakesget_window(time), withapodizationtyped to it so a source profile is rejected at construction.Continuous-mode scaling becomes the window's coherent gain
2 / sum(w), which reduces to the previous2 / Nfor the rectangular case. The sum is taken over the recorded steps, so this composes withdft_subsample.Summary by CodeRabbit
WindowProfileimplementations:GaussianWindowProfileandTukeyWindowProfile.PhasorDetectorviaWindowProfile, including coherent-gain correction for continuous recordings.WindowProfile,GaussianWindowProfile, andTukeyWindowProfile.