Fix Nyquist bin inclusion for even N FFT lengths#71
Conversation
Introduces CLAUDE.md to document architecture, development commands, testing strategy, and configuration for the spectral_connectivity Python package. This file provides guidance for contributors and AI assistants working with the codebase.
Problem: - The non-negative frequency indexing used `arange(0, (N+1)//2)` which incorrectly excludes the Nyquist frequency bin for even N FFT lengths - For even N=1024: old formula gives 512 bins, missing the Nyquist bin - For odd N=1023: old formula gives 512 bins (correct, no Nyquist exists) Solution: - Replace `arange(0, (N+1)//2)` with `arange(0, N//2 + 1)` - For even N=1024: new formula gives 513 bins (includes Nyquist) - For odd N=1023: new formula gives 512 bins (same as before) Changes: - Updated _non_negative_frequencies decorator in connectivity.py - Updated canonical_coherence method frequency indexing - Updated _estimate_spectral_granger_prediction function - Added unit tests for both even and odd N cases - Updated wrapper test to expect correct frequency count This fix ensures proper FFT frequency bin handling according to standard conventions where even N includes the Nyquist frequency. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect frequency indexing for even-length FFTs by ensuring the Nyquist frequency bin is properly included. The current implementation uses (N+1)//2 which works for odd N but excludes the Nyquist bin for even N, affecting connectivity measure accuracy.
Key changes:
- Updated frequency indexing formula from
(N+1)//2toN//2 + 1in three locations - Added comprehensive test coverage for both even and odd FFT lengths
- Updated existing test expectations to include the Nyquist frequency
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spectral_connectivity/connectivity.py | Fixed frequency indexing in decorator, canonical coherence, and Granger prediction methods |
| tests/test_connectivity.py | Added dedicated tests for Nyquist bin inclusion validation |
| tests/test_wrapper.py | Updated frequency expectations to include Nyquist bin |
| CLAUDE.md | Added new documentation file for repository guidance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Test that frequencies property shape matches when provided | ||
| # (frequencies is None when not provided to constructor) | ||
|
|
||
|
|
There was a problem hiding this comment.
There are extra blank lines between the incomplete comment and the next function definition. Remove the extra blank line on line 632 to maintain consistent spacing.
| # Test that frequencies property shape matches when provided | ||
| # (frequencies is None when not provided to constructor) |
There was a problem hiding this comment.
The comment about testing frequencies property shape is incomplete and doesn't correspond to any actual test code. Either complete the test implementation or remove the orphaned comment.
Document the breaking change in frequency bin indexing for even-length FFTs. Includes impact analysis, migration guidance, and scientific justification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Problem
The current non-negative frequency indexing implementation incorrectly excludes the Nyquist frequency bin for even N FFT lengths. This affects the accuracy of connectivity measures computed on even-length FFTs.
Current Behavior
arange(0, (N+1)//2)for non-negative frequency selectionMathematical Issue
According to FFT conventions:
N//2 + 1non-negative frequencies (DC + positive + Nyquist)(N+1)//2non-negative frequencies (DC + positive, no Nyquist)The current formula
(N+1)//2works for odd N but misses the Nyquist bin for even N.Solution
Replace
arange(0, (N+1)//2)witharange(0, N//2 + 1)which correctly handles both cases:1024//2 + 1 = 513frequency bins ✅ (includes Nyquist)1023//2 + 1 = 512frequency bins ✅ (same as(1023+1)//2)Changes Made
Core Changes
_non_negative_frequenciesdecorator (line 107): Updated frequency indexing logiccanonical_coherencemethod (line 638): Fixed frequency selection for canonical coherence_estimate_spectral_granger_prediction(line 2108): Fixed Granger causality frequency indexingTest Changes
test_nyquist_bin_even_n(): Validates N=1024 produces 513 frequenciestest_nyquist_bin_odd_n(): Validates N=1023 produces 512 frequenciestest_frequencies(): Expects correct frequency count including NyquistValidation
Test Results
Frequency Verification
For
n_fft_samples=4,sampling_frequency=1000Hz:[0, 250]Hz (2 frequencies, missing Nyquist)[0, 250, -500]Hz (3 frequencies, includes Nyquist at -500 Hz)Regression Testing
Impact
This fix ensures:
The implementation follows test-driven development principles with comprehensive validation of both even and odd N cases.
🤖 Generated with Claude Code