Enhance DSP Core: Vectorization, IEC Standards, and Precision#35
Enhance DSP Core: Vectorization, IEC Standards, and Precision#35
Conversation
- Fix RMS calculation in core.py (use sqrt(mean(x^2))) - Implement asymmetric IEC 61672-1 time constants for 'impulse' weighting - Vectorize core processing for efficient multichannel support - Update Makefile to use pytest - Improve filter visualization with dynamic frequency ticks - Update README and generate new compliance graphs
Summary of ChangesHello @jmrplens, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly enhances the library's capabilities. The vectorization of the core processing logic for multichannel signals is a major performance improvement. The implementation of proper IEC-compliant impulse time weighting and the refinement of RMS calculations increase the precision and correctness of the library. The accompanying updates to documentation and plotting scripts are also well-executed. I have two minor suggestions for code simplification and a performance consideration for the new impulse weighting feature. Overall, great work.
| elif mode_lower == "impulse": | ||
| # IEC 61672-1: 35ms for rising, 1500ms for falling | ||
| tau_rise = 0.035 | ||
| tau_fall = 1.5 | ||
|
|
||
| alpha_rise = 1 - np.exp(-1 / (fs * tau_rise)) | ||
| alpha_fall = 1 - np.exp(-1 / (fs * tau_fall)) | ||
|
|
||
| # Iterate over time (last axis) to handle asymmetric logic | ||
| # Move time axis to front for iteration | ||
| x_t = np.moveaxis(x_sq, -1, 0) | ||
| y_t = np.zeros_like(x_t) | ||
|
|
||
| # Initialize state (channels shape) | ||
| curr_y = np.zeros(x_t.shape[1:]) | ||
|
|
||
| # Simple loop over time samples | ||
| for i, val in enumerate(x_t): | ||
| # Vectorized over channels | ||
| rising = val > curr_y | ||
| diff = val - curr_y | ||
| curr_y += np.where(rising, alpha_rise, alpha_fall) * diff | ||
| y_t[i] = curr_y | ||
|
|
||
| # Move time axis back | ||
| return np.moveaxis(y_t, 0, -1) | ||
|
|
There was a problem hiding this comment.
The implementation of impulse weighting with asymmetric rise/fall times is correct according to IEC 61672-1. However, the use of a Python for loop to iterate over time samples can lead to poor performance on long audio signals compared to vectorized scipy.signal functions. For real-time or large file processing, this could become a bottleneck.
Consider exploring ways to optimize this, for example by using a JIT compiler like Numba. Adding a @numba.jit decorator to a slightly modified version of this function could provide a significant speedup with minimal code changes, though it would introduce a new dependency.
| if current_length > target_length: | ||
| # Slice along the last axis | ||
| if y_resampled.ndim == 1: | ||
| y_resampled = y_resampled[:target_length] | ||
| else: | ||
| y_resampled = y_resampled[..., :target_length] | ||
|
|
||
| elif current_length < target_length: | ||
| diff = target_length - current_length | ||
| if y_resampled.ndim == 1: | ||
| pad_width = (0, diff) | ||
| else: | ||
| # Pad only the last axis | ||
| pad_width = [(0, 0)] * (y_resampled.ndim - 1) + [(0, diff)] | ||
|
|
||
| y_resampled = np.pad(y_resampled, pad_width, mode='constant') | ||
|
|
There was a problem hiding this comment.
The logic to handle both 1D and 2D arrays for slicing and padding is correct. However, it can be simplified by removing the if y_resampled.ndim == 1: checks, as the logic in the else blocks already handles the 1D case correctly. Using ... for slicing and constructing pad_width with y_resampled.ndim works universally for arrays of one or more dimensions, making the code more concise and easier to maintain.
if current_length > target_length:
# Slice along the last axis (works for both 1D and 2D)
y_resampled = y_resampled[..., :target_length]
elif current_length < target_length:
diff = target_length - current_length
# Pad only the last axis. This works for both 1D and 2D arrays.
# For 1D, pad_width becomes `[(0, diff)]`.
# For 2D, pad_width becomes `[(0, 0), (0, diff)]`.
pad_width = [(0, 0)] * (y_resampled.ndim - 1) + [(0, diff)]
y_resampled = np.pad(y_resampled, pad_width, mode='constant')- Fix mypy errors in core.py and utils.py - Add tests/test_coverage_fix.py to restore 100% coverage - Verify all checks pass
- Remove unused pytest import in test_coverage_fix.py - Simplify _resample_to_length logic in utils.py
CI Results 🚀Test Summary
Filter Architecture Benchmark ReportThis report compares the performance and characteristics of the available filter types. 1. Spectral Isolation (at 1kHz)
2. Stability and Performance
3. Analysis Summary
Generated Graphs |
- Add numba dependency - Implement JIT-compiled kernel for asymmetric time weighting loop - Maintain pure Python fallback for robustness
- Update requires-python to >=3.11 in pyproject.toml - Remove 3.9 and 3.10 from CI matrix - Set mypy target version to 3.11
- Set Python 3.11+ as minimum version - Remove legacy fallbacks for typing.Literal - Make numba a mandatory dependency and optimize impulse kernel - Remove pure Python fallback for impulse weighting - Refactor octavefilter API to be explicit (no more **kwargs) - Add __version__ to package and __repr__ to OctaveFilterBank - Consolidate coverage tests and achieve 100% traceable coverage
- Fix module-level import order in parametric_filters.py - Fix mypy errors (redundant casts, Any returns) - Remove unused imports - Achieve clean ruff and mypy state
CI Results 🚀Test Summary
Filter Architecture Benchmark ReportThis report compares the performance and characteristics of the available filter types. 1. Spectral Isolation (at 1kHz)
2. Stability and Performance
3. Analysis Summary
Generated Graphs |
- Implement WeightingFilter class for pre-calculating A/C/Z coefficients - Update weighting_filter to use the new class - Update calculate_sensitivity to use real RMS formula for consistency - Ensure all tests pass with 100% traceable coverage
CI Results 🚀Test Summary
Filter Architecture Benchmark ReportThis report compares the performance and characteristics of the available filter types. 1. Spectral Isolation (at 1kHz)
2. Stability and Performance
3. Analysis Summary
Generated Graphs |
- Add explicit type signatures and caching to Numba impulse kernel - Double check and confirm utils.py simplicity according to PR suggestions - Final verification of codebase consistency
…graphs - Fix: remove explicit signature from _apply_impulse_kernel to support both 1D and 2D arrays automatically - Regenerate all visualization assets including updated time weighting analysis - Final verification of tests, linting and type checking
CI Results 🚀Test Summary
Filter Architecture Benchmark ReportThis report compares the performance and characteristics of the available filter types. 1. Spectral Isolation (at 1kHz)
2. Stability and Performance
3. Analysis Summary
Generated Graphs |
- Move 'ripple' and 'attenuation' to **kwargs - Maintain explicit documentation and default values - Ensure total parameters count is 13
CI Results 🚀Test Summary
Filter Architecture Benchmark ReportThis report compares the performance and characteristics of the available filter types. 1. Spectral Isolation (at 1kHz)
2. Stability and Performance
3. Analysis Summary
Generated Graphs |
… parameters - Update sonar-project.properties to ignore S107 in __init__.py - Restore explicit 'ripple' and 'attenuation' parameters in octavefilter - Ensure high usability and discoverability of the main API
CI Results 🚀Test Summary
Filter Architecture Benchmark ReportThis report compares the performance and characteristics of the available filter types. 1. Spectral Isolation (at 1kHz)
2. Stability and Performance
3. Analysis Summary
Generated Graphs |
- Add passband ripple validation (central 80%) - Add group delay variance (phase linearity) analysis - Add crossover flatness evaluation - Enhance multichannel performance metrics - Add technical recommendations per filter architecture
- Silenced scipy signal warnings for cleaner console output - Integrated visual charts into benchmark report - Added executive summary and methodology sections - Tracked new benchmark visualization assets
- Unified graphs and benchmark generation in 'assets' job - Implemented auto-amend and force-push for asset synchronization - Cleaned up redundant deployment steps
- Added tests/conftest.py to disable Numba JIT during tests automatically - Ensured JIT is re-enabled for performance tests - Added 'coverage' target to Makefile - Updated CI environment for explicit coverage support
1eae280 to
596cbeb
Compare
f54cf73 to
a7c5880
Compare
CI Results 🚀Test Summary
No images generated. |
528b818 to
e591d18
Compare
CI Results 🚀Test Summary
No images generated. |
817bbbf to
64f00de
Compare
64f00de to
8bba7de
Compare
- Avoid shell=True in benchmark script for B602 compliance - Decouple pr-comment job from assets to prevent skip in PRs - Ensure CI comments are posted even on failure for visibility
CI Results 🚀Test Summary
No images generated. |
- Use absolute path for grep in benchmark script - Implement [skip ci] logic in assets job to avoid infinite workflow recursion - Preserve original commit messages during asset amendments
16cec15 to
b291fc5
Compare
CI Results 🚀Test Summary
No images generated. |
CI Results 🚀Test Summary
No images generated. |
|































































































































































No description provided.