fix: real-domain spectrum renders as +-fc conjugate-symmetric Euler pairs - #40
Merged
Merged
Conversation
striderZA
commented
Aug 4, 2026
striderZA
left a comment
Owner
Author
There was a problem hiding this comment.
Review: PR #40 — Real-domain spectrum Euler/conjugate-symmetry fix
Verified locally (worktree .worktrees/spectrum-euler, commit 2f0b561):
- Full build: clean (
cmake --build build, no warnings from touched files). build/bin/tests.exe: 217 test cases / 65522 assertions, all pass — matches PR claim, no regressions.build/bin/test_signal_domain.exe: 24 test cases / 46 assertions, all pass — matches PR claim.
Design review:
- The core physics is correct:
conjugateSymmetricExpandsplits non-DC tones into±fcpairs atP - 10*log10(2)dBm each, DC passes through unchanged. Confirmed against Euler's formula. - Scoping decision to keep interior DSP (generator,
nonlinear_model.h, mixer, gain/filter/S-param stages) on the collapsed single-entry representation is correct and well-justified — splitting there would break the amplitude-calibrated harmonic/IM math. Render-time-only expansion is the right call even though issue #39's "Expected behaviour" text nominally also asks for the representation to change; the design doc's §2 rationale for not doing that is sound. - ADC power bookkeeping analysis (§5 of the design doc) checks out: since interior tones are never actually split before reaching the ADC, no compensation gain is needed on the DDC output — adding one would double-count and break the existing "ADC DDC preserves tone power and phase" regression test. Confirmed this test still passes unmodified.
is_complex_basebandpropagation was checked line-by-line against everyupdate()branch inamplifier,attenuator,combiner,equalizer,ideal_filter,coax,splitter,mixer,pfb_channelizer,touchstone/s_parameter_data.cpp. Coverage is complete for every branch that assignsout.tones.renderCombinedSpectrumreuses the fixedintegratePowerPerBin, so multi-spectrum overlay rendering picks up the fix for free — no separate gap there.- Frequency grids for all real-domain producers span
[MIN_FREQ, MAX_FREQ] = [-20GHz, +20GHz](buildDefaultFrequencyGrid/ signal generator), so the-fcmirror always has a bin to land in; no silent-drop risk from a positive-only grid.
Minor, non-blocking findings:
combiner_engine.cpp's twoN < 2degenerate-frequency-grid early-return branches (lines ~103 and ~198) exit before the newout.is_complex_basebandassignment runs, so the flag can go stale in that edge case. This is not a new bug — it's exact parity with the pre-existingout.fs_Hzgap in the same branches (also unset there today). Not worth blocking on; flag iffs_Hzstaleness there is ever fixed, fixis_complex_basebandalongside it.- Merge conflict with current
master:masterhas since merged #38 which also appends a new test executable block at the end oftests/CMakeLists.txt(test_issue37_pfb_input_removal), colliding textually with this PR'stest_signal_domainblock at the same insertion point. Trivial rebase (keep bothadd_executable/add_testblocks), but the PR needs it before merge — GitHub reportsMERGEABLE: DIRTY.
Verdict: Approve pending a rebase onto current master to resolve the tests/CMakeLists.txt conflict. No functional or physics concerns.
Split (-3.0103dB) + DDC compensation (+3.0103dB) cancel to net zero. Interior tones are never actually split, so the ADC's existing unchanged pass-through of power_dBm already IS the compensated result. Adding an explicit +3.0103dB (as drafted) would have double-applied the compensation and broken the ADC DDC regression test.
…est-registration ceiling
…for complex-baseband spectra
striderZA
force-pushed
the
fix/spectrum-euler-conjugate-symmetry
branch
from
August 4, 2026 18:08
2f0b561 to
ca3feda
Compare
Owner
Author
|
Rebased onto current master ( |
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
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.
Summary
Fixes #39. In the "real" (analog) signal domain — everywhere upstream of the ADC's DDC — the spectrum display now correctly honors Euler's formula: a real cosine at
fcis physically0.5*(exp(j*2*pi*fc*t) + exp(-j*2*pi*fc*t)), i.e. equal-power content at both+fcand-fc. The spectrum-analyzer render path previously collapsed this to one full-power bin at+fcwith no representation of the mirror.Design
Full design doc:
docs/superpowers/specs/2026-08-03-spectrum-euler-conjugate-symmetry-design.mdImplementation plan:
docs/superpowers/plans/2026-08-03-spectrum-euler-conjugate-symmetry.mdSpectrum::is_complex_basebandflag (defaultfalse), propagated through every pass-through component exactly likefs_Hzalready is.conjugateSymmetricExpand()helper incommon/spectrum.h: splits each non-DC tone into+fc/-fchalf-power (-3.0103 dB) entries per Euler's formula. DC tones pass through unchanged.nonlinear_model.h's harmonic/IM math, every gain/filter/S-parameter stage, and the mixer keep operating on the existing collapsed single-entry-per-tone representation — splitting there would corrupt the nonlinear model's real-power-calibrated harmonic math.is_complex_baseband == false) spectra.AdcEngine's output now setsis_complex_baseband = true. Its DDC power/phase math (alias_frequency, NCO-shift/windowing) is unchanged — verified against the repo's owndocs/resources/rf_adc_info.md, image rejection there already correctly discards the mirror, so no additional compensation is needed.Testing
New standalone executable
tests/test_signal_domain.cpp(required by this toolchain's MinGW-w64 TEST_CASE registration ceiling — seetests/AGENTS.md), 24 test cases / 46 assertions, covering:is_complex_basebandflag defaulting and propagation through every component (generator, amplifier, attenuator, combiner, equalizer, ideal filter, coax, splitter, mixer, PFB channelizer, touchstone S-parameter data, ADC).conjugateSymmetricExpand()unit tests (real tone split, DC tone passthrough, multiple tones, empty input).+/-fchalf-power pair; a complex-baseband tone renders unchanged (no mirroring).Full existing suite (
build/bin/tests.exe) verified unchanged throughout every commit: 217 test cases / 65522 assertions, zero regressions.Commits
7 implementation tasks + design/plan docs + a DOX pass (updated
common/AGENTS.mdandtests/AGENTS.mdper this repo's binding documentation contract) + one review-driven perf fix (avoid an unnecessary tone-vector copy in the common complex-baseband render path).