feat: add IEC 61260-1 nominal frequency labels#46
Merged
Conversation
Reviewer's GuideAdds opt-in IEC 61260-1 nominal frequency label support across the octave filter API, extending frequency generation to return labels, wiring them through OctaveFilterBank and the octavefilter wrapper, and adding helpers/tests for nominal rounding and formatting. Sequence diagram for octavefilter nominal label flowsequenceDiagram
actor User
participant OctaveFilterFunction as octavefilter
participant FilterBank as OctaveFilterBank
participant FreqGen as _genfreqs
participant ANSI as getansifrequencies
User->>OctaveFilterFunction: octavefilter(x, fs, ..., nominal=True)
OctaveFilterFunction->>FilterBank: __init__(limits, fraction, fs)
FilterBank->>FreqGen: _genfreqs(limits, fraction, fs)
FreqGen->>ANSI: getansifrequencies(fraction, limits)
ANSI-->>FreqGen: freq, freq_d, freq_u, labels
FreqGen-->>FilterBank: freq, freq_d, freq_u, labels
OctaveFilterFunction->>FilterBank: filter(x, sigbands, mode, detrend, calculate_level, nominal=True)
FilterBank->>FilterBank: compute SPL bands
FilterBank->>FilterBank: freq_out = nominal_freq
FilterBank-->>OctaveFilterFunction: spl, nominal_freq[, sigbands]
OctaveFilterFunction-->>User: spl, List[str] nominal labels[, sigbands]
Class diagram for octave filter API with IEC 61260-1 nominal labelsclassDiagram
class OctaveFilterBank {
+List[float] freq
+List[float] freq_d
+List[float] freq_u
+List[str] nominal_freq
+__init__(limits: List[float], fraction: float, fs: int, order: int, stateful: bool)
+filter(x: List[float] | np.ndarray, sigbands: bool, mode: str, detrend: bool, calculate_level: bool, nominal: bool) Tuple
+_process_bands(x: List[float] | np.ndarray, sigbands: bool, mode: str, detrend: bool, calculate_level: bool) Tuple
}
class FrequenciesModule {
+getansifrequencies(fraction: float, limits: List[float] | None) Tuple[List[float], List[float], List[float], List[str]]
+_genfreqs(limits: List[float], fraction: float, fs: int) Tuple[List[float], List[float], List[float], List[str]]
+_iec_e3_round(f: float) float
+_nominal_freq_for_band(exact_freq: float, fraction: float) float
+_format_nominal_freq(f: float) str
+_deleteouters(freq: List[float], freq_d: List[float], freq_u: List[float], fs: int) Tuple[List[float], List[float], List[float]]
+normalizedfreq(fraction: int) List[float]
}
class OctaveFilterFunction {
+octavefilter(x: List[float] | np.ndarray, fs: int, fraction: float, order: int, limits: List[float] | None, show: bool, sigbands: bool, plot_file: str | None, detrend: bool, filter_type: str, ripple: float, attenuation: float, calibration_factor: float, dbfs: bool, mode: str, nominal: bool) Tuple
}
OctaveFilterBank ..> FrequenciesModule : uses _genfreqs
OctaveFilterFunction ..> OctaveFilterBank : creates
OctaveFilterFunction ..> OctaveFilterBank : calls filter(nominal)
FrequenciesModule ..> FrequenciesModule : getansifrequencies calls _nominal_freq_for_band
FrequenciesModule ..> FrequenciesModule : _nominal_freq_for_band calls normalizedfreq
FrequenciesModule ..> FrequenciesModule : _nominal_freq_for_band calls _iec_e3_round
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
3 tasks
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds IEC 61260-1 nominal frequency label support: frequency generators now return nominal labels; OctaveFilterBank stores Changes
Sequence DiagramsequenceDiagram
participant User
participant octavefilter as "octavefilter()"
participant FilterBank as "OctaveFilterBank.filter()"
participant FreqGen as "FrequencyGeneration"
User->>octavefilter: call(signal, nominal=True)
octavefilter->>FilterBank: filter(signal, ..., nominal=True)
FilterBank->>FreqGen: request bands (compute center, edges, nominal labels)
FreqGen-->>FilterBank: (centers, lower, upper, nominal_labels)
FilterBank-->>octavefilter: (ndarray, nominal_labels, sigbands?)
octavefilter-->>User: return result with nominal labels
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello, 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
Changelog
Activity
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing
getansifrequenciesfrom a 3-tuple to a 4-tuple is a breaking API change for any external callers; consider either adding a new helper for labeled frequencies or making the additional labels optional (e.g., via a flag or a small data structure) to preserve the original return signature. - In
_genfreqs, the logic assumes_deleteoutersonly drops trailing bands and then sliceslabelsaccordingly; if_deleteoutersbehavior ever changes (e.g., removes bands in the middle), labels will desynchronize—consider having_deleteoutersreturn the indices or operate on a single structure so labels and frequencies stay aligned by construction. - The return type annotations for
filter/octavefilterare becoming quite complex with large unions; consider introducing a type alias or narrowing overloads (e.g., grouping bynominal/sigbands) to keep the public type surface easier to read and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `getansifrequencies` from a 3-tuple to a 4-tuple is a breaking API change for any external callers; consider either adding a new helper for labeled frequencies or making the additional labels optional (e.g., via a flag or a small data structure) to preserve the original return signature.
- In `_genfreqs`, the logic assumes `_deleteouters` only drops trailing bands and then slices `labels` accordingly; if `_deleteouters` behavior ever changes (e.g., removes bands in the middle), labels will desynchronize—consider having `_deleteouters` return the indices or operate on a single structure so labels and frequencies stay aligned by construction.
- The return type annotations for `filter`/`octavefilter` are becoming quite complex with large unions; consider introducing a type alias or narrowing overloads (e.g., grouping by `nominal`/`sigbands`) to keep the public type surface easier to read and maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a great new feature for displaying IEC 61260-1 nominal frequency labels. The implementation is solid, with good use of Python's typing features for clarity and correctness. The new functionality is also well-covered by a new suite of tests. I've found a couple of opportunities for minor refactoring to improve maintainability and performance. Overall, this is a high-quality contribution.
src/pyoctaveband/__init__.py
Outdated
Comment on lines
+193
to
+196
| if sigbands: | ||
| return filter_bank.filter(x, sigbands=True, mode=mode, detrend=detrend) | ||
| return filter_bank.filter(x, sigbands=True, mode=mode, detrend=detrend, nominal=nominal) # type: ignore[call-overload,no-any-return] | ||
| else: | ||
| return filter_bank.filter(x, sigbands=False, mode=mode, detrend=detrend) No newline at end of file | ||
| return filter_bank.filter(x, sigbands=False, mode=mode, detrend=detrend, nominal=nominal) # type: ignore[call-overload,no-any-return] |
There was a problem hiding this comment.
This if/else block can be simplified into a single return statement by passing the sigbands parameter directly. This reduces code duplication and improves readability.
return filter_bank.filter(x, sigbands=sigbands, mode=mode, detrend=detrend, nominal=nominal) # type: ignore[call-overload,no-any-return]
Comment on lines
+141
to
+153
| def _nominal_freq_for_band(exact_freq: float, fraction: float) -> float: | ||
| """Return IEC 61260-1 nominal frequency (float) for an exact mid-band frequency. | ||
|
|
||
| For standard fractions (1, 3), snaps to the IEC preferred table via | ||
| ``normalizedfreq``. For non-standard fractions, falls back to Annex E.3 | ||
| significant-figure rounding (``_iec_e3_round``). | ||
| """ | ||
| freq, freq_d, freq_u = getansifrequencies(fraction, limits) | ||
| return _deleteouters(freq, freq_d, freq_u, fs) | ||
| frac = round(fraction) | ||
| if frac in (1, 3): | ||
| base = normalizedfreq(frac) | ||
| extended: List[float] = [f * (10 ** d) for d in range(-3, 4) for f in base] | ||
| return min(extended, key=lambda f: abs(np.log(f / exact_freq))) | ||
| return _iec_e3_round(exact_freq) |
There was a problem hiding this comment.
The extended list of preferred frequencies is re-calculated for every frequency band inside getansifrequencies. This is inefficient. You can cache this list as it only depends on the fraction. Here is a suggestion using a function attribute as a simple cache, which avoids re-computation without needing module-level variables or new imports.
Suggested change
| def _nominal_freq_for_band(exact_freq: float, fraction: float) -> float: | |
| """Return IEC 61260-1 nominal frequency (float) for an exact mid-band frequency. | |
| For standard fractions (1, 3), snaps to the IEC preferred table via | |
| ``normalizedfreq``. For non-standard fractions, falls back to Annex E.3 | |
| significant-figure rounding (``_iec_e3_round``). | |
| """ | |
| freq, freq_d, freq_u = getansifrequencies(fraction, limits) | |
| return _deleteouters(freq, freq_d, freq_u, fs) | |
| frac = round(fraction) | |
| if frac in (1, 3): | |
| base = normalizedfreq(frac) | |
| extended: List[float] = [f * (10 ** d) for d in range(-3, 4) for f in base] | |
| return min(extended, key=lambda f: abs(np.log(f / exact_freq))) | |
| return _iec_e3_round(exact_freq) | |
| def _nominal_freq_for_band(exact_freq: float, fraction: float) -> float: | |
| """Return IEC 61260-1 nominal frequency (float) for an exact mid-band frequency. | |
| For standard fractions (1, 3), snaps to the IEC preferred table via | |
| ``normalizedfreq``. For non-standard fractions, falls back to Annex E.3 | |
| significant-figure rounding (``_iec_e3_round``). | |
| """ | |
| frac = round(fraction) | |
| if frac in (1, 3): | |
| if not hasattr(_nominal_freq_for_band, "_cache"): | |
| _nominal_freq_for_band._cache = {} # type: ignore | |
| if frac not in _nominal_freq_for_band._cache: | |
| base = normalizedfreq(frac) | |
| _nominal_freq_for_band._cache[frac] = [f * (10 ** d) for d in range(-3, 4) for f in base] | |
| extended = _nominal_freq_for_band._cache[frac] | |
| return min(extended, key=lambda f: abs(np.log(f / exact_freq))) | |
| return _iec_e3_round(exact_freq) |
Contributor
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 06:54:00 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pyoctaveband/__init__.py`:
- Around line 173-175: The docstring return type currently only documents
float-based frequency lists; update the :rtype: to also include the nominal
(List[str]) variants when nominal=True. Specifically, amend the Union so it
includes Tuple[np.ndarray, List[str]] and Tuple[np.ndarray, List[str],
List[np.ndarray]] alongside the existing float-based tuples (referencing the
nominal parameter and the function returning (SPL_array, Frequencies_list) or
(SPL_array, Frequencies_list, signals)). Ensure both 2-tuple and 3-tuple forms
are represented in the :rtype: line.
In `@src/pyoctaveband/frequencies.py`:
- Around line 14-24: The public API of getansifrequencies was changed to return
a 4-tuple which breaks existing callers; update the package/runtime version to
1.2.0 to signal the breaking change and include a clear changelog entry and
release note referencing getansifrequencies so users are aware of the new return
shape, or alternatively restore backward compatibility by providing a wrapper
overload that returns a 3-tuple for older callers and mark it deprecated. Ensure
the version bump (to 1.2.0), changelog entry, and any deprecation comment
reference getansifrequencies and the new 4-tuple return.
- Around line 141-153: The function _nominal_freq_for_band incorrectly uses
round(fraction) so non-integer fractions like 0.6 or 2.6 are coerced into 1 or 3
and routed to the preferred-table lookup; change the logic to only treat
fraction as an integer when it is within a tiny tolerance of an integer (e.g.,
abs(fraction - round(fraction)) < 1e-8) and then cast to int and check if that
int is in (1, 3) before calling normalizedfreq(frac); otherwise fall back to
_iec_e3_round(exact_freq). Ensure you reference _nominal_freq_for_band,
normalizedfreq, and _iec_e3_round when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0caf2489-bf83-40bc-85f9-8a66be02d534
📒 Files selected for processing (5)
src/pyoctaveband/__init__.pysrc/pyoctaveband/core.pysrc/pyoctaveband/frequencies.pytests/test_coverage_fix.pytests/test_nominal_frequencies.py
Comment on lines
14
to
24
| def getansifrequencies( | ||
| fraction: float, | ||
| limits: List[float] | None = None, | ||
| ) -> Tuple[List[float], List[float], List[float]]: | ||
| ) -> Tuple[List[float], List[float], List[float], List[str]]: | ||
| """ | ||
| Calculate frequencies according to ANSI/IEC standards. | ||
|
|
||
| :param fraction: Bandwidth fraction (e.g., 1, 3). | ||
| :param limits: [f_min, f_max] limits. | ||
| :return: Tuple of (center_freqs, lower_edges, upper_edges). | ||
| :return: Tuple of (center_freqs, lower_edges, upper_edges, nominal_labels). | ||
| """ |
There was a problem hiding this comment.
Treat the 4-tuple return as a versioned API break.
This changes a public unpacking contract. Please pair it with the planned 1.2.0 runtime/package version bump before release; otherwise existing callers still on 1.1.x will fail with too many values to unpack.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pyoctaveband/frequencies.py` around lines 14 - 24, The public API of
getansifrequencies was changed to return a 4-tuple which breaks existing
callers; update the package/runtime version to 1.2.0 to signal the breaking
change and include a clear changelog entry and release note referencing
getansifrequencies so users are aware of the new return shape, or alternatively
restore backward compatibility by providing a wrapper overload that returns a
3-tuple for older callers and mark it deprecated. Ensure the version bump (to
1.2.0), changelog entry, and any deprecation comment reference
getansifrequencies and the new 4-tuple return.
Co-authored-by: ninoblumer <40544174+ninoblumer@users.noreply.github.com>
jmrplens
force-pushed
the
feature/nominal-freq-labels-v2
branch
from
07bbf89 to
4e75c18
Compare
Contributor
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 10:12:38 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pyoctaveband/frequencies.py (1)
156-160:⚠️ Potential issue | 🟡 MinorUse an explicit tiny tolerance for standard fractions.
np.isclosewith default tolerances still treats values like1.00001as equal to1, so some non-standard fractions can still be routed through the preferred-table lookup instead of the Annex E.3 fallback.Possible fix
- frac = round(fraction) - if np.isclose(fraction, frac) and frac in (1, 3): + frac = round(fraction) + if abs(fraction - frac) < 1e-8 and frac in (1, 3): extended = _extended_preferred(frac) return min(extended, key=lambda f: abs(np.log(f / exact_freq)))Verify the current behavior with a small read-only check;
fraction=1.00001andfraction=3.00003should currently printisclose=True, which demonstrates the gap:#!/bin/bash python - <<'PY' import numpy as np for fraction in [1.0, 1.000001, 1.00001, 0.99999, 3.0, 3.00003]: frac = round(fraction) print( f"fraction={fraction:<8} rounded={frac} " f"isclose={np.isclose(fraction, frac)} " f"abs_diff={abs(fraction - frac)}" ) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pyoctaveband/frequencies.py` around lines 156 - 160, The existing check using np.isclose(fraction, frac) is too permissive; update the condition in the block that computes frac = round(fraction) (and checks frac in (1, 3) before calling _extended_preferred) to use an explicit tiny tolerance, e.g. np.isclose(fraction, frac, atol=1e-6) (or a similarly small absolute tolerance) so values like 1.00001/3.00003 don't pass as "close" and will correctly fall through to _iec_e3_round(exact_freq).
🧹 Nitpick comments (1)
tests/test_coverage_fix.py (1)
142-143: Assert the new 4-tuple contract, not just unpackability.This only proves the extra value can be unpacked. It will still pass if
labelsis the wrong length or drifts out of sync with the generated bands, which is the key behavior added here.Suggested test tightening
f1, f2, f3, labels = getansifrequencies(1, limits=None) assert len(f1) > 0 + assert len(f1) == len(f2) == len(f3) == len(labels) + assert all(isinstance(label, str) for label in labels)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_coverage_fix.py` around lines 142 - 143, The test only verifies unpacking of getansifrequencies(1, limits=None) into f1, f2, f3, labels but not the intended 4-tuple contract; update the assertions in tests/test_coverage_fix.py to validate the new contract by asserting labels has the expected length and alignment with the returned frequency bands from getansifrequencies (e.g., ensure labels is a sequence, len(labels) matches the number of bands implied by f1/f2/f3 or the combined frequency arrays, and that label indices correspond to the generated bands), referencing getansifrequencies and the variables f1, f2, f3, labels to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pyoctaveband/frequencies.py`:
- Around line 156-160: The existing check using np.isclose(fraction, frac) is
too permissive; update the condition in the block that computes frac =
round(fraction) (and checks frac in (1, 3) before calling _extended_preferred)
to use an explicit tiny tolerance, e.g. np.isclose(fraction, frac, atol=1e-6)
(or a similarly small absolute tolerance) so values like 1.00001/3.00003 don't
pass as "close" and will correctly fall through to _iec_e3_round(exact_freq).
---
Nitpick comments:
In `@tests/test_coverage_fix.py`:
- Around line 142-143: The test only verifies unpacking of getansifrequencies(1,
limits=None) into f1, f2, f3, labels but not the intended 4-tuple contract;
update the assertions in tests/test_coverage_fix.py to validate the new contract
by asserting labels has the expected length and alignment with the returned
frequency bands from getansifrequencies (e.g., ensure labels is a sequence,
len(labels) matches the number of bands implied by f1/f2/f3 or the combined
frequency arrays, and that label indices correspond to the generated bands),
referencing getansifrequencies and the variables f1, f2, f3, labels to locate
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90990015-4639-4e1e-8ff8-b92328ef8950
📒 Files selected for processing (6)
sonar-project.propertiessrc/pyoctaveband/__init__.pysrc/pyoctaveband/core.pysrc/pyoctaveband/frequencies.pytests/test_coverage_fix.pytests/test_nominal_frequencies.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_nominal_frequencies.py
- Bump version to 1.2.0 (breaking: getansifrequencies returns 4-tuple) - Sync __init__.py __version__ with pyproject.toml - Simplify sigbands dispatch in __init__.py (single return) - Update docstring :rtype: with nominal label variants - Use np.isclose() for fraction matching in frequencies.py - Cache extended preferred frequency list with @lru_cache - Add sonar.cpd.exclusions for overload duplication - Tighten test assertions for 4-tuple contract
jmrplens
force-pushed
the
feature/nominal-freq-labels-v2
branch
from
4e75c18 to
ac4faa8
Compare
|
Contributor
CI Results 🚀Test Summary
Technical Benchmark Summary📊 View Benchmark DetailsPyOctaveBand: Technical Benchmark ReportGenerated: 2026-03-08 10:36:09 1. Test Signal Parameters
2. Crossover (Linkwitz-Riley)
3. Precision & Isolation
4. Performance
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rebased and refined version of #44 (by @ninoblumer) — adds IEC 61260-1 nominal frequency labels to octave/fractional-octave band analysis.
Changes from original PR #44
core.pyoverloads (8 combined overloads for sigbands × calculate_level × nominal)import pytest, renamed ambiguousl→label(E741)__init__.pyoverloads: addedLiteral[True]nominal variants returningList[str]_genfreqs()now reuses labels fromgetansifrequencies()via slice instead of recomputing_nominal_freq_for_bandexplaining fallback for non-standard fractionsextendedlist, addedtype: ignorefor overload dispatch in wrapperFeatures (from #44)
nominal=Trueparameter onOctaveFilterBank.filter()andoctavefilter()"1k","31.5","12.5k") instead of numeric frequenciesgetansifrequencies()now returns 4-tuple including labelsOctaveFilterBank.nominal_freqproperty for direct label accessCI Status
Closes #44
Summary by Sourcery
Add optional IEC 61260-1 nominal frequency labels to octave and fractional-octave band analysis outputs.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit