Fix what the analyzer reports, and stop hiding the rest from it - #506
Merged
Conversation
SonarCloud had 420 open issues and a configuration that silenced some of what it would otherwise have said. Both halves are dealt with here, and nothing is suppressed: the exclusions are gone and no new ones replace them. ## The suppressions sonar-project.properties carried seven S107 exemptions (functions with more than thirteen parameters) and two duplication exclusions; filters/core.py carried five inline NOSONAR comments. All of them are removed. What they hid is fixed instead. One of the seven was already dead: it named src/phonometry/__init__.py, where no function takes more than thirteen parameters at all. ## The wide signatures Every function S107 fired on now groups its related parameters into a small frozen dataclass, so no signature exceeds the limit and no functionality is lost. The groups follow each standard's own structure rather than an arbitrary split, and every default is preserved exactly. The clearest case: the same six air parameters (speed of sound, density, viscosity, Prandtl number, heat-capacity ratio, atmospheric pressure) were repeated across eight functions in two modules. They are one AirProperties now, with DEFAULT_AIR as the shared default. The physics was verified against the previous implementation over twelve output sets, with default and non-default air, both resonator geometries and oblique incidence: bit-identical. The worst signature in the library was predicted_receiver_level, which took sixteen positional parameters. Sixteen bare values in a row means transposing two of them produces a wrong answer rather than an error. OctaveFilterBank keeps the four parameters everyone passes (fs, fraction, limits, order) exactly as they were, positionally and by name, so OctaveFilterBank(48000, 3) still reads the same; only the advanced options a minority passes are grouped. ## The 420 issues The two biggest rules are both about tests and both worth doing. S5778 (151) wanted a single call inside each pytest.raises block, so the exception being asserted comes from the function under test and not from the construction of its arguments. S9073 (124) wanted composite assertions split, so a failure says which half failed. That first rule found a test that did not test what it claimed: test_task_rejects_nonpositive_duration asserted that task_based_exposure rejects a zero duration, but Task.__post_init__ rejects it first, so the function under test was never reached. Its own duration guard is unreachable for any real Task and has no coverage. S1192 (77) became named constants, S125 (20) removed commented-out code (except where the comment was an ISO 9612 table row index that merely parses as Python, now reworded), and S3776 (19) extracted helpers where the complexity was incidental and left it where the complexity is the physics. ## The ISO notation T1..T4 and T_a/T_b in the scattering module are lower case now. The project already writes standard symbols that way (lp_aeqt, ln_w_eq, la_max, dnt), so these were the exception, which is why S117 fired on them and nothing else. The correspondence with ISO 17497-1 is as clear as it is for every other symbol, and the conformance check now reads t1=t1 where it read T1=t1. ## Two repairs the above required The test that guards the Sonar configuration asserted that exemptions exist. It accepts none now, and still catches an entry left pointing at a moved file the moment one is added. make llms could not run in any checkout with local notes under docs/: it required a site page for every markdown file it found. It skips what git ignores now.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @jmrplens, your pull request is larger than the review limit of 150000 diff characters
|
You've reached the fair usage limit for the Bito Code Review Agent trial. Upgrade to a paid plan or contact support@bito.in for assistance. Learn more |
|
Important Review skippedToo many files! This PR contains 273 files, which is 173 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (273)
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
documentation
Reviewer's GuideRefactors several high-parameter scientific APIs into small frozen dataclasses, removes SonarCloud suppressions by extracting helpers and tightening validation, and improves plotting/i18n/docs consistency while keeping numerical behaviour unchanged. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
+ Coverage 97.00% 97.04% +0.04%
==========================================
Files 295 295
Lines 38787 39148 +361
==========================================
+ Hits 37626 37992 +366
+ Misses 1161 1156 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Numerical conformance report✅ 533/533 conformance checks pass across 57 domains and 362 standards - filters class 1 - weightings within IEC 61672-1 class 1. Each row pins a standard clause to its expected normative value and the value the library computes. Every section below is collapsible and stays collapsed while all of its rows pass; a section with any failing row opens automatically. ✅ Numerical validation - filters & weightings: class showcase (IEC 61260-1 · IEC 61672-1 · ISO 7196)IEC 61260-1:2014 class per filter architecture (order 6, one-third-octave, 100 Hz-10 kHz, fs = 48 kHz). For each architecture the table shows, at its binding band, the measured relative attenuation and the class-1 limit it must clear, so the number and the range it must sit in are both visible. A positive margin means the acceptance limits are met with that much room.
Only Butterworth (the library default) and Chebyshev-II are class-compliant architectures. Chebyshev-I and elliptic trade the mask for passband ripple, and Bessel for a maximally-flat group delay (soft rolloff); they cannot satisfy the IEC 61260-1 Class 1/2 attenuation mask by construction, so they are labelled By design - this is expected, not a failure or regression. Frequency-weighting conformance (A/C: IEC 61672-1 Table 3; G: ISO 7196 A.3). The max deviation from nominal is informational (it falls at a frequency extreme where the tolerance is widest and asymmetric); compliance is judged at the binding frequency - the one with the least headroom - where the deviation, the applicable tolerance band and the headroom are shown together.
✅ Filters & weightings: 100% (10/10)
✅ Levels & dosimetry: 100% (9/9)
✅ Room & building acoustics: 100% (72/72)
✅ Room acoustics: 100% (16/16)
✅ Psychoacoustics: 100% (14/14)
✅ Speech transmission (IEC 60268-16): 100% (10/10)
✅ System measurement (Golay / Kirkeby / Mueller-Massarani): 100% (5/5)
✅ Intensity & sound power: 100% (10/10)
✅ Building prediction & uncertainty: 100% (15/15)
✅ Outdoor propagation & occupational exposure: 100% (10/10)
✅ Materials: absorption, airflow & impedance: 100% (6/6)
✅ Scattering & diffusion (ISO 17497): 100% (14/14)
✅ In-situ road absorption (ISO 13472): 100% (3/3)
✅ Precision sound power (ISO 3745 / 9614-3): 100% (4/4)
✅ Human vibration (ISO 8041 / 2631 / 5349): 100% (15/15)
✅ Speech intelligibility (ANSI S3.5-1997): 100% (24/24)
✅ Objective intelligibility (STOI / ESTOI): 100% (3/3)
✅ Impulsive-sound prominence (NT ACOU 112): 100% (2/2)
✅ Impulsive-sound prominence (ISO/PAS 1996-3): 100% (2/2)
✅ Room noise (ANSI S12.2-2019): 100% (3/3)
✅ Hearing threshold (ISO 7029 / ISO 389-7): 100% (3/3)
✅ Measurement uncertainty (GUM / Supplement 1): 100% (7/7)
✅ Noise-induced hearing loss (ISO 1999): 100% (6/6)
✅ Multiple-shock whole-body vibration (ISO 2631-5): 100% (6/6)
✅ Sound absorption in enclosed spaces (EN 12354-6): 100% (2/2)
✅ Prominent discrete tones (ECMA-418-1): 100% (2/2)
✅ Tonal audibility (ISO/PAS 20065): 100% (11/11)
✅ Psychoacoustic annoyance & fluctuation strength (Fastl & Zwicker): 100% (3/3)
✅ Electroacoustics: distortion & frequency response: 100% (20/20)
✅ Calibrated spectral analysis (Bendat & Piersol): 100% (12/12)
✅ Multiple-input coherence (Bendat & Piersol): 100% (5/5)
✅ Time-frequency analysis (Bendat & Piersol): 100% (3/3)
✅ Correlation, time delay and envelope (B&P / Knapp & Carter): 100% (7/7)
✅ Cepstrum, liftering and envelope spectrum (Havelock / B&P): 100% (3/3)
✅ Time synchronous averaging (McFadden 1987): 100% (5/5)
✅ Data qualification and Rice statistics (Bendat & Piersol): 100% (8/8)
✅ Underwater acoustics (ISO 18405/17208/18406): 100% (6/6)
✅ Underwater sound propagation (transmission loss): 100% (16/16)
✅ Underwater propagation regimes (Weston flux theory): 100% (3/3)
✅ Marine-mammal auditory weighting (NMFS / Southall): 100% (4/4)
✅ Underwater numerical propagation (modes / rays / PE): 100% (4/4)
✅ Aircraft noise (ICAO Annex 16 / IEC 61265): 100% (15/15)
✅ Rotorcraft noise (ECAC Doc 32 / NORAH2): 100% (12/12)
✅ CNOSSOS-EU road source (Directive 2002/49/EC Annex II): 100% (6/6)
✅ Wind-turbine noise (IEC 61400-11): 100% (3/3)
✅ Porous & multilayer absorbers (Mechel / Bies / Cox & D'Antonio): 100% (20/20)
✅ Slow-sound perfect absorbers (Jimenez et al. Appl. Sci. 2017): 100% (3/3)
✅ Program loudness (ITU-R BS.1770 / EBU R 128): 100% (8/8)
✅ 2D FDTD wave simulation (Attenborough & Van Renterghem 2021, Ch. 4): 100% (4/4)
✅ Swept-sine distortion & phase utilities (Farina / Novak): 100% (7/7)
✅ Spherical ground & barriers (Attenborough / Salomons / Bies): 100% (7/7)
✅ Panel & aperture sound insulation (Bies / Hopkins / Cremer): 100% (17/17)
✅ Bending-wave plate-junction transmission (Cremer / Craik / Hopkins): 100% (6/6)
✅ Atmospheric refraction (Salomons rays / GFPE): 100% (3/3)
✅ Electroacoustics: 100% (9/9)
✅ Industrial noise control: 100% (22/22)
✅ CNOSSOS-EU railway source (Directive 2002/49/EC Annex II): 100% (8/8)
Tests & coverage — 48204 tests, 0 failures (✅ all green)
Conformance harness: |
Grouping the wide parameter lists turned one call into two, and where that call
sat inside a `pytest.raises` block the bundle construction went in with it:
with pytest.raises(ValueError, match="scalar"):
rotorcraft_event_level(hems, spd, ang, t, pos, (0.0, 0.0),
ground=RotorcraftGround(flow_resistivity=sigma))
Two calls in the block means the test no longer proves which one raised, which
is what S5778 exists to catch. It flagged 33 of these on the pull request, all
of them in files this branch had just touched, plus one function whose
cognitive complexity the same change pushed over the limit.
Every bundle involved is a plain frozen data holder with no `__post_init__`, so
in all 33 cases the raiser is the function under test and hoisting the
construction out of the block is the correct fix. Where a bundle had validated,
the fix would have been the opposite: keep it inside and hoist the rest.
The complexity one is `elastic_fdtd_simulation`, at 17 against a limit of 15.
Two validators come out of it, `_validated_snapshot_options` and
`_validated_absorbing_layer`, in the style of the helpers already in that file.
Validation order is unchanged, so which error fires first is unchanged, and the
conformance report still prints the same numbers.
|
|
You've reached the fair usage limit for the Bito Code Review Agent trial. Upgrade to a paid plan or contact support@bito.in for assistance. Learn more |
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.
SonarCloud had 420 open issues, and a configuration that stopped it reporting
some of what it would otherwise have said. This deals with both halves, and
suppresses nothing: the exclusions are gone and none replace them. If anything
turns out to genuinely need one, it gets its own branch and its own argument.
The suppressions are gone
sonar-project.propertiescarried sevenS107exemptions (functions overthirteen parameters) and two duplication exclusions;
filters/core.pycarriedfive inline
NOSONARcomments. All removed, and what they hid is fixed ratherthan re-hidden.
One of the seven was already dead: it named
src/phonometry/__init__.py, whereno function takes more than thirteen parameters at all. It had been silencing
nothing for some time.
The wide signatures are fixed, not excused
Every function
S107fired on now bundles its related parameters into a smallfrozen dataclass. No signature is over the limit, no functionality is lost, and
each grouping follows the structure of the standard it implements rather than
an arbitrary split. Every default is preserved exactly.
rotorcraft_noise_contourrotorcraft_event_levelmicrophone_characteristicsloudspeaker_characteristicspredicted_receiver_levelelastic_fdtd_simulationslit_helmholtz_absorbersound_power_pressuremetadiffuser_polar_responseOctaveFilterBank.__init__The clearest case is the air state. The same six parameters (speed of sound,
density, viscosity, Prandtl number, heat-capacity ratio, atmospheric pressure)
were repeated across eight functions in two modules. They are one
AirPropertiesnow, withDEFAULT_AIRas the shared default, living besidethe constants it defaults from. The physics was checked against the previous
implementation over twelve output sets, with default and non-default air, both
resonator geometries, both correction branches and oblique incidence:
bit-identical.
The worst signature was
predicted_receiver_level, which took sixteenpositional parameters. Sixteen bare values in a row means transposing two of
them yields a wrong answer instead of an error, which is precisely what the
rule exists to catch.
OctaveFilterBankkeeps the four parameters callers actually use (fs,fraction,limits,order) exactly as they were, positionally and by name,so
OctaveFilterBank(48000, 3)is unchanged. Only the advanced options aminority passes are grouped; the counts that decided this came from parsing all
85 call sites.
The 420 issues
The two largest rules are both about tests, and both are worth doing on their
merits.
S5778(151) wants one call inside eachpytest.raisesblock, so the exceptionasserted comes from the function under test rather than from building its
arguments.
S9073(124) wants composite assertions split, so a failure tellsyou which half failed.
The first found a test that did not test what it claimed:
test_task_rejects_nonpositive_durationasserted thattask_based_exposurerejects a zero duration, but
Task.__post_init__rejects it first, so thefunction under test was never reached. Its own duration guard is unreachable
for any real
Taskand has no coverage. The test now exercises what it names;the dead guard is left alone as a maintainer decision rather than removed here.
S1192(77) became named constants,S125(20) removed commented-out code,and
S3776(19) extracted helpers where the complexity was incidental and leftit where the complexity is the physics. One
S125group was not dead code atall: the ISO 9612 Table C.4 row indices (
# N=3) merely parse as Python, andare reworded rather than deleted.
ISO notation
T1..T4andT_a/T_bin the scattering module are lower case now. Theproject already writes standard symbols that way (
lp_aeqt,ln_w_eq,la_max,dnt,lp_as_4m), so these were the exception, which is whyS117fired on them and on nothing else. The correspondence with ISO 17497-1 reads as
clearly as it does for every other symbol, and the conformance check now says
t1=t1where it saidT1=t1.Two repairs this required
The test guarding the Sonar configuration asserted that exemptions exist. It
accepts none now, while still catching an entry left pointing at a moved file
the moment one is added back.
make llmscould not run in any checkout holding local notes underdocs/: itdemanded a site page for every markdown file it found. It skips what git
ignores now.
Checks
8041 tests,
ruff,mypyover 419 files, 533/533 conformance checks withdocs/CONFORMANCE.mdunchanged, the 70 example fiches byte for byte (theIEC 60268 signatures that feed them changed), 3663 documentation snippets over
508 pages, the API reference regenerated at 1297 public names, and the llms
artifacts.
The count SonarCloud reports on this branch is the real measure, and it now
measures everything, with nothing excluded.