Skip to content

Fix IndexError in THD() for low/fractional fundamental frequencies - #42

Open
youdie006 wants to merge 1 commit into
endolith:masterfrom
youdie006:fix/38-thd-harmonic-index-bounds
Open

Fix IndexError in THD() for low/fractional fundamental frequencies#42
youdie006 wants to merge 1 commit into
endolith:masterfrom
youdie006:fix/38-thd-harmonic-index-bounds

Conversation

@youdie006

@youdie006 youdie006 commented Aug 20, 2026

Copy link
Copy Markdown

Repro

THD() raises IndexError when the fundamental is low enough that its FFT bin rounds up:

import numpy as np
from waveform_analysis import THD

fs, N = 12000, 12000
t = np.arange(N) / fs
sig = np.sin(2 * np.pi * 6 * t)
THD(sig, fs, freq=5.994)
    ampl = abs(f[i * h])
IndexError: index 6006 is out of bounds for axis 0 with size 6001

This is the case described in #38 (fundamental low, rounded bin on the wrong side).

Root cause

THD() computes the number of harmonics from the continuous fundamental:

num_harmonics = int((fs/2)/frequency)

but indexes each harmonic with the rounded fundamental bin i = int(round(frequency * N / fs)):

for h in range(2, num_harmonics + 1):
    ampl = abs(f[i * h])

When the fractional bin rounds up, i * num_harmonics overshoots the rfft length len(f) = N//2 + 1, so the last harmonic's bin lands past Nyquist and the index is out of bounds. In the repro, i = round(5.994) = 6, num_harmonics = 1001, and i * 1001 = 6006 > 6001.

Fix

Guard the index before use and stop the loop:

if i * h >= len(f):
    break

Physics oracle: a harmonic at bin >= len(f) corresponds to a frequency >= fs/2, above Nyquist, which cannot exist in the sampled spectrum, so it must be excluded. break (not continue) is correct because h increases monotonically -- once one harmonic is out of range, every later one is too. Harmonics whose bin is in range are summed exactly as before, so results for normal signals are unchanged (existing test_sine/test_sawtooth/test_freq_parameter still pass).

Test

Added a regression test that reproduces #38 and asserts a finite THD:

def test_low_fundamental_no_indexerror(self):
    fs = 12000
    signal = sine_wave(6, fs)
    result = THD(signal, fs, freq=5.994)
    assert np.isfinite(result)
    assert result >= 0

Before the fix it raises IndexError: index 6006 is out of bounds for axis 0 with size 6001; after, it returns a finite ratio.

Thanks to @khaut for reporting.


AI-assisted: this change was prepared with the help of an AI coding assistant and reviewed by me.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where THD calculations could fail for low-frequency signals due to out-of-range harmonic analysis.
    • THD analysis now safely stops at the available frequency range, including frequencies above Nyquist.
  • Tests

    • Added regression coverage confirming low, fractional frequencies produce finite, nonnegative THD results.

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2d04ce12-c10d-49e2-8974-5ae569700b1a

📥 Commits

Reviewing files that changed from the base of the PR and between baece1e and 1d0b4ce.

📒 Files selected for processing (2)
  • tests/test_thd.py
  • waveform_analysis/thd.py

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

THD now stops harmonic analysis before it accesses FFT bins beyond the spectrum. A regression test covers a low fractional frequency that rounds to an FFT-bin boundary.

Changes

THD harmonic bounds

Layer / File(s) Summary
Bounded harmonic analysis and regression coverage
waveform_analysis/thd.py, tests/test_thd.py
THD stops when the harmonic-bin index reaches the FFT result length. The regression test verifies a finite, nonnegative result for a low fractional fundamental frequency.

Estimated code review effort: 2 (Simple) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 1d0b4

The change prevents THD() from indexing harmonics beyond the Nyquist limit while preserving in-range calculations, with a focused regression test covering the failure case. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the fix for the IndexError in THD() caused by low or fractional fundamental frequencies.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

THD() computes the harmonic count from the continuous fundamental
(num_harmonics = int((fs/2)/frequency)) but indexes each harmonic with the
rounded fundamental bin i = int(round(frequency*N/fs)). When the fractional bin
rounds up, i * num_harmonics overshoots the rfft length len(f) = N//2+1, so the
last harmonic's bin lands past Nyquist and f[i*h] is out of bounds. For a low
fundamental (e.g. THD(sig, 12000, freq=5.994): i=6, num_harmonics=1001,
i*1001=6006 > 6001) this raises IndexError.

Guard the index and break the loop once i*h reaches len(f). A harmonic at bin
>= len(f) is above the Nyquist frequency and cannot exist in the sampled
spectrum, so it must be excluded; break (not continue) is correct because h
increases monotonically. Harmonics in range are summed exactly as before, so
results for normal signals are unchanged.

Fixes endolith#38.
@youdie006
youdie006 force-pushed the fix/38-thd-harmonic-index-bounds branch from 1d0b4ce to 647d8d8 Compare August 25, 2026 10:48
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.44%. Comparing base (6bfa59b) to head (647d8d8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   80.34%   80.44%   +0.09%     
==========================================
  Files          12       12              
  Lines         407      409       +2     
==========================================
+ Hits          327      329       +2     
  Misses         80       80              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant