Skip to content

Frequency Analysis: prose & comment clarity (xcorr amplitude scaling, magnitude_spectrum *2, bin-spacing vs resolution) #24

Description

@jonfroehlich

Found in the #16 correctness/prose/comments audit (each verified by running). The DSP math in Signals - Frequency Analysis.ipynb is correct; these are clarity/accuracy improvements on the explanatory text and comments (per the audit's prose + comment dimensions).

  1. Cross-correlation amplitude scaling is coupled to a default argument. In calc_and_plot_xcorr_dft_with_ground_truth, the y-axis amplitude is computed as (correlation_results / (nyquist_limit - 1)) * 2. This only recovers the true amplitude because the comparison-signal length in samples happens to ≈ nyquist_limit - 1 with the default args; with a shorter xcorr_comparison_signal_length_in_secs (an exposed parameter) the amplitude axis silently goes wrong (verified: 0.25 s → reads 0.5× the true amplitude). Normalize by the comparison length in samples (len(signal_to_test)), or add a comment stating the assumption.

  2. Uncommented magnitude_spectrum(s * 2, ...). plt.magnitude_spectrum returns half-amplitude for a real sine, so the signal is pre-scaled by 2 to make the peak read the true amplitude — but there's no comment, and an earlier cell documents the opposite convention (scale the returned spectrum, not the input). Add a one-line comment so students aren't misled.

  3. "Bin spacing = resolution" needs a caveat. A cell states the FFT bin spacing is the frequency resolution, but the notebook elsewhere zero-pads (n = len(s)*2/3/4), which shrinks bin spacing without improving true resolution (a later cell correctly warns about this). Qualify the claim: bin spacing equals resolution only when the FFT length equals the signal length (no zero-padding).

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentation

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions