-
-
Notifications
You must be signed in to change notification settings - Fork 19
Detailed API reference table with parameters and returns #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 improves the documentation within 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
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great step towards improving the API documentation in the README.md. The new detailed table is much more user-friendly. However, my review found several critical discrepancies between the new documentation and the actual function signatures in the source code. These inaccuracies could lead to confusion and incorrect usage of the library. I've provided specific comments and suggestions to align the documentation with the implementation, particularly for calculate_sensitivity, getansifrequencies, and normalizedfreq, which have significant errors. Addressing these will make the API reference accurate and reliable.
| | `weighting_filter` | `function` | **Acoustic weighting.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `curve`: 'A', 'C', or 'Z' (Default: 'A') | `y = weighting_filter(x, fs, curve='A')`<br><br>• `y`: 1D array of weighted signal | | ||
| | `time_weighting` | `function` | **Energy capture.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `mode`: 'fast', 'slow', or 'impulse' | `env = time_weighting(x, fs, mode='fast')`<br><br>• `env`: 1D array of energy envelope (Mean Square) | | ||
| | `linkwitz_riley` | `function` | **Audio crossover.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `freq`: Crossover frequency [Hz]<br>• `order`: 4 or 8 (Default: 4) | `lo, hi = linkwitz_riley(x, fs, freq=1000, order=4)`<br><br>• `lo`: Low-pass filtered signal<br>• `hi`: High-pass filtered signal | | ||
| | `calculate_sensitivity` | `function`| **SPL Calibration.**<br>• `x_ref`: Calibration signal<br>• `fs`: Sample rate [Hz]<br>• `target_spl`: Level of calibrator (Default: 94.0) | `s = calculate_sensitivity(x_ref, fs, target_spl=94.0)`<br><br>• `s`: Float (multiplier for pressure) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a critical mismatch between the documentation for calculate_sensitivity and its actual implementation in the source code.
- Incorrect Parameters: The function signature is
calculate_sensitivity(ref_signal, target_spl=94.0, ref_pressure=2e-5). It does not take anfsparameter. - Missing Parameter: The
ref_pressureparameter is not documented. - Inconsistent Naming: The documentation uses
x_refwhile the code usesref_signal.
I recommend rewriting this entry to accurately reflect the function's signature and purpose.
| | `calculate_sensitivity` | `function`| **SPL Calibration.**<br>• `x_ref`: Calibration signal<br>• `fs`: Sample rate [Hz]<br>• `target_spl`: Level of calibrator (Default: 94.0) | `s = calculate_sensitivity(x_ref, fs, target_spl=94.0)`<br><br>• `s`: Float (multiplier for pressure) | | |
| | `calculate_sensitivity` | `function`| **SPL Calibration.**<br>• `ref_signal`: Calibration signal array<br>• `target_spl`: Level of calibrator [dB] (Default: 94.0)<br>• `ref_pressure`: Reference pressure [Pa] (Default: 2e-5) | `s = calculate_sensitivity(ref_signal, target_spl=94.0)`<br><br>• `s`: Float (sensitivity multiplier) | |
| | `time_weighting` | `function` | **Energy capture.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `mode`: 'fast', 'slow', or 'impulse' | `env = time_weighting(x, fs, mode='fast')`<br><br>• `env`: 1D array of energy envelope (Mean Square) | | ||
| | `linkwitz_riley` | `function` | **Audio crossover.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `freq`: Crossover frequency [Hz]<br>• `order`: 4 or 8 (Default: 4) | `lo, hi = linkwitz_riley(x, fs, freq=1000, order=4)`<br><br>• `lo`: Low-pass filtered signal<br>• `hi`: High-pass filtered signal | | ||
| | `calculate_sensitivity` | `function`| **SPL Calibration.**<br>• `x_ref`: Calibration signal<br>• `fs`: Sample rate [Hz]<br>• `target_spl`: Level of calibrator (Default: 94.0) | `s = calculate_sensitivity(x_ref, fs, target_spl=94.0)`<br><br>• `s`: Float (multiplier for pressure) | | ||
| | `getansifrequencies` | `function` | **ANSI Frequency generator.**<br>• `fraction`: 1, 3, etc. (Default: 1)<br>• `limits`: [f_min, f_max] (Default: [12, 20000]) | `f = getansifrequencies(fraction=3, limits=[20, 20000])`<br><br>• `f`: List of center frequencies [Hz] | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for getansifrequencies has a couple of critical inaccuracies:
- No Default for
fraction: Thefractionparameter is required and does not have a default value of 1. - Incorrect Return Value: The function returns a tuple of three lists:
(center_frequencies, lower_band_edges, upper_band_edges). The documentation incorrectly states it only returns a single list of center frequencies.
The usage snippet f = getansifrequencies(...) is also misleading, as it would only capture the first of the three return values. This should be corrected to freq, freq_d, freq_u = getansifrequencies(...) to reflect the actual output.
| | `getansifrequencies` | `function` | **ANSI Frequency generator.**<br>• `fraction`: 1, 3, etc. (Default: 1)<br>• `limits`: [f_min, f_max] (Default: [12, 20000]) | `f = getansifrequencies(fraction=3, limits=[20, 20000])`<br><br>• `f`: List of center frequencies [Hz] | | |
| | `getansifrequencies` | `function` | **ANSI Frequency generator.**<br>• `fraction`: 1, 3, etc.<br>• `limits`: [f_min, f_max] (Default: [12, 20000]) | `freq, freq_d, freq_u = getansifrequencies(fraction=3, limits=[20, 20000])`<br><br>• `freq`: List of center frequencies [Hz]<br>• `freq_d`: List of lower edge frequencies [Hz]<br>• `freq_u`: List of upper edge frequencies [Hz] | |
| | `linkwitz_riley` | `function` | **Audio crossover.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `freq`: Crossover frequency [Hz]<br>• `order`: 4 or 8 (Default: 4) | `lo, hi = linkwitz_riley(x, fs, freq=1000, order=4)`<br><br>• `lo`: Low-pass filtered signal<br>• `hi`: High-pass filtered signal | | ||
| | `calculate_sensitivity` | `function`| **SPL Calibration.**<br>• `x_ref`: Calibration signal<br>• `fs`: Sample rate [Hz]<br>• `target_spl`: Level of calibrator (Default: 94.0) | `s = calculate_sensitivity(x_ref, fs, target_spl=94.0)`<br><br>• `s`: Float (multiplier for pressure) | | ||
| | `getansifrequencies` | `function` | **ANSI Frequency generator.**<br>• `fraction`: 1, 3, etc. (Default: 1)<br>• `limits`: [f_min, f_max] (Default: [12, 20000]) | `f = getansifrequencies(fraction=3, limits=[20, 20000])`<br><br>• `f`: List of center frequencies [Hz] | | ||
| | `normalizedfreq` | `function` | **Band edge calculator.**<br>• `freq`: Center frequency [Hz]<br>• `fraction`: 1, 3, etc. | `f_low, f_up = normalizedfreq(1000, 3)`<br><br>• `f_low`: Lower cut-off frequency [Hz]<br>• `f_up`: Upper cut-off frequency [Hz] | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation entry for normalizedfreq is incorrect and describes a function that doesn't match the source code.
The actual normalizedfreq(fraction) function takes a single integer argument (1 or 3) and returns a predefined list of standard IEC center frequencies for that fraction. It does not calculate band edges.
The current description, parameters, and return values seem to belong to a different (or non-existent) function. This entire row should be rewritten to accurately describe what normalizedfreq does.
| | `normalizedfreq` | `function` | **Band edge calculator.**<br>• `freq`: Center frequency [Hz]<br>• `fraction`: 1, 3, etc. | `f_low, f_up = normalizedfreq(1000, 3)`<br><br>• `f_low`: Lower cut-off frequency [Hz]<br>• `f_up`: Upper cut-off frequency [Hz] | | |
| | `normalizedfreq` | `function` | **Standard Frequencies.**<br>• `fraction`: 1 or 3 | `freq = normalizedfreq(fraction=3)`<br><br>• `freq`: List of standard IEC center frequencies [Hz] | |
| | `calculate_sensitivity` | `function`| Calibrate digital signals to physical SPL. | `s = calculate_sensitivity(ref, target_spl=94)` | | ||
| | `getansifrequencies` | `function` | Generate preferred ANSI frequencies. | `f = getansifrequencies(fraction=3)` | | ||
| | `normalizedfreq` | `function` | Compute band edge frequencies. | `f_low, f_up = normalizedfreq(1000, 3)` | | ||
| | `octavefilter` | `function` | **High-level analysis.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `fraction`: 1, 3, etc. (Default: 1)<br>• `order`: Filter order (Default: 6)<br>• `filter_type`: 'butter', 'cheby1', 'cheby2', 'ellip', 'bessel' (Default: 'butter')<br>• `sigbands`: Return time signals (Default: False) | `spl, freq = octavefilter(x, fs, ...)`<br>• `spl`: levels [dB]<br>• `freq`: frequencies [Hz]<br><br>**With `sigbands=True`:**<br>`spl, freq, xb = octavefilter(x, fs, sigbands=True)`<br>• `xb`: List of filtered signals (one per band) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for octavefilter is missing several important parameters that are available through **kwargs. For completeness and user clarity, I recommend adding them.
Missing parameters include:
limits: Frequency limits[f_min, f_max].ripple: Passband ripple forcheby1andellipfilters.attenuation: Stopband attenuation forcheby2andellipfilters.calibration_factor: For physical SPL calculation.dbfs: To get results in dBFS.mode: To select'rms'or'peak'level calculation.
Additionally, when sigbands=True, the function returns a third value: a list of the time-domain signals for each band. This should also be documented in the 'Outputs' section.
| | `getansifrequencies` | `function` | Generate preferred ANSI frequencies. | `f = getansifrequencies(fraction=3)` | | ||
| | `normalizedfreq` | `function` | Compute band edge frequencies. | `f_low, f_up = normalizedfreq(1000, 3)` | | ||
| | `octavefilter` | `function` | **High-level analysis.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `fraction`: 1, 3, etc. (Default: 1)<br>• `order`: Filter order (Default: 6)<br>• `filter_type`: 'butter', 'cheby1', 'cheby2', 'ellip', 'bessel' (Default: 'butter')<br>• `sigbands`: Return time signals (Default: False) | `spl, freq = octavefilter(x, fs, ...)`<br>• `spl`: levels [dB]<br>• `freq`: frequencies [Hz]<br><br>**With `sigbands=True`:**<br>`spl, freq, xb = octavefilter(x, fs, sigbands=True)`<br>• `xb`: List of filtered signals (one per band) | | ||
| | `OctaveFilterBank` | `class` | **Efficient bank implementation.**<br>• `fs`: Sample rate [Hz]<br>• `fraction`: 1, 3, etc.<br>• `order`: Filter order<br>• `limits`: [f_min, f_max] (Default: [12, 20000])<br>• `filter_type`: Architecture name | `bank = OctaveFilterBank(fs=48000, fraction=3, order=6, filter_type='butter')`<br>`spl, f = bank.filter(x)`<br><br>• `bank`: Instance of the filter bank | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation entry is a bit confusing. The 'Outputs' section describes the bank object itself, but the usage example correctly shows that the useful output comes from the bank.filter(x) method.
I suggest two improvements:
- Add the missing parameters for the
OctaveFilterBankconstructor, such asripple,attenuation,dbfs, etc., along with their defaults. - Change the 'Outputs' section to describe the return values of
bank.filter(x), which aresplandfreq, similar to how it's done for theoctavefilterfunction. This would make the documentation much clearer for the user.
| | `octavefilter` | `function` | **High-level analysis.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `fraction`: 1, 3, etc. (Default: 1)<br>• `order`: Filter order (Default: 6)<br>• `filter_type`: 'butter', 'cheby1', 'cheby2', 'ellip', 'bessel' (Default: 'butter')<br>• `sigbands`: Return time signals (Default: False) | `spl, freq = octavefilter(x, fs, ...)`<br>• `spl`: levels [dB]<br>• `freq`: frequencies [Hz]<br><br>**With `sigbands=True`:**<br>`spl, freq, xb = octavefilter(x, fs, sigbands=True)`<br>• `xb`: List of filtered signals (one per band) | | ||
| | `OctaveFilterBank` | `class` | **Efficient bank implementation.**<br>• `fs`: Sample rate [Hz]<br>• `fraction`: 1, 3, etc.<br>• `order`: Filter order<br>• `limits`: [f_min, f_max] (Default: [12, 20000])<br>• `filter_type`: Architecture name | `bank = OctaveFilterBank(fs=48000, fraction=3, order=6, filter_type='butter')`<br>`spl, f = bank.filter(x)`<br><br>• `bank`: Instance of the filter bank | | ||
| | `weighting_filter` | `function` | **Acoustic weighting.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `curve`: 'A', 'C', or 'Z' (Default: 'A') | `y = weighting_filter(x, fs, curve='A')`<br><br>• `y`: 1D array of weighted signal | | ||
| | `time_weighting` | `function` | **Energy capture.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `mode`: 'fast', 'slow', or 'impulse' | `env = time_weighting(x, fs, mode='fast')`<br><br>• `env`: 1D array of energy envelope (Mean Square) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mode parameter has a default value that isn't mentioned in the documentation. To make the API reference more complete, please consider adding (Default: 'fast') to its description.
| | `time_weighting` | `function` | **Energy capture.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `mode`: 'fast', 'slow', or 'impulse' | `env = time_weighting(x, fs, mode='fast')`<br><br>• `env`: 1D array of energy envelope (Mean Square) | | |
| | `time_weighting` | `function` | **Energy capture.**<br>• `x`: Signal array<br>• `fs`: Sample rate [Hz]<br>• `mode`: 'fast', 'slow', or 'impulse' (Default: 'fast') | `env = time_weighting(x, fs, mode='fast')`<br><br>• `env`: 1D array of energy envelope (Mean Square) | |
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 |
|























































This PR enhances the "Quick API Reference" section in the
README.md:<br>) within the table to maintain a clean and readable layout.