Repository navigation
Results that were tuples are named records, tolerance limits come lower first, and a record's required fields refuse to be left empty - #978
Open
jmrplens wants to merge 1 commit into
Conversation
|
Sorry @jmrplens, your pull request is larger than the review limit of 150,000 diff characters |
|
Important Review skippedToo many files! This PR contains 221 files, which is 121 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 configuration
⛔ Files ignored due to path filters: 10
📒 Files selected for processing: 221
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 |
jmrplens
added this pull request to stack #984
October 17, 2026 17:11
jmrplens
changed the title
jmrplens
force-pushed
the
fix/records-instead-of-tuples
branch
from
October 17, 2026 17:12
b066ab3 to
0c39c61
Compare
github-actions
Bot
added
documentation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/verdicts-spell-passes-and-refuse-truth #978 +/- ##
=============================================================================
Coverage ? 96.91%
=============================================================================
Files ? 445
Lines ? 90579
Branches ? 0
=============================================================================
Hits ? 87782
Misses ? 2797
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Numerical conformance
1893/1893 checks pass across 111 domains and 523 standards (227 normative designations, 125 further published sources). Used in the tables below is how much of that clause's published tolerance the deviation consumes: 100 % sits exactly on the limit, 5 % uses a twentieth of the allowance, and a dash means the clause states no two-sided tolerance for the quantity, so there is no budget to spend. It is reported and never used to decide a verdict, which is settled at full precision before any rounding. Renamed (4)
Closest to their published limit (top 5) The rows with the least room left, so the ones a change is most likely to push over.
|
| Python Version | Tests | Failures | Coverage | Status |
|---|---|---|---|---|
| macos-latest-3.12 | 29012 | 0 | 96.9% |  Passed |
| macos-latest-3.13 | 29012 | 0 | 96.9% | Passed |
| macos-latest-3.14 | 29012 | 0 | 96.9% | Passed |
| ubuntu-latest-3.12 | 29012 | 0 | 96.9% | Passed |
| ubuntu-latest-3.13 | 29012 | 0 | 96.9% | Passed |
| ubuntu-latest-3.14 | 29012 | 0 | 96.9% | Passed |
| windows-latest-3.12 | 29012 | 0 | 96.9% | Passed |
| windows-latest-3.13 | 29012 | 0 | 96.9% | Passed |
| windows-latest-3.14 | 29012 | 0 | 96.9% | Passed |
Full report at this commit: docs/CONFORMANCE.md · docs/conformance.json · full CI artifacts
…er first, and a record's required fields refuse to be left empty Four answers that came back as a tuple to be unpacked by position are records with named fields now. `OctaveFilterBank.spectrogram` returns a `filters.OctaveSpectrogramResult` with `levels`, a read-only `frequencies` and `times`; it checks the level grid against both axes and draws itself with `.plot()`, one raster over time and the logarithmic band centre with a dB colour bar, and a multichannel result asks for `channel=`. `filters.nominal_frequencies` returns a `filters.FractionalOctaveBands` with read-only `frequencies`, `lower_edges` and `upper_edges` and the IEC 61260-1 nominal `labels` as a tuple of text; the filter bank reads its band set from the same record, as `bank.frequencies`, `bank.lower_edges`, `bank.upper_edges`, `bank.labels` and `bank.n_bands`, all read-only. `building.improvement_octave_bands` and the `octave_bands()` methods of the floor-covering, laboratory lining, laboratory floor-covering, joint and rainfall results return a `building.OctaveBandSpectrum` with `frequencies`, `values` in dB and the `quantity` they hold, which plots itself in English or Spanish under the symbol its source prints. `aircraft.epnl_from_pnlt` returns an `aircraft.EPNLIntegration` with `epnl`, `pnltm`, `bandsharing_adjustment`, `band_limits` and the derived `duration_correction`, and `effective_perceived_noise_level` builds its `EPNLResult` from it.
jmrplens
force-pushed
the
fix/records-instead-of-tuples
branch
from
October 20, 2026 01:45
0c39c61 to
075ac5e
Compare
 [Medium impact] Refactors result types from tuples to named records across multiple domains. The PR appears safe to merge, with non-blocking fixes needed for single-band plotting and missing octave-spectrum columns. FindingsReviews (1) · Last reviewed commit: "Results that were tuples are named recor..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1187
to
+1188
| else: | ||
| half_step = math.sqrt(2.0) |
There was a problem hiding this comment.
For OctaveFilterBank(48000, fraction=3, limits=[990, 1010]), the 1000 Hz band has edges around 891–1122 Hz. Its spectrogram instead spans 707–1414 Hz because this branch always uses sqrt(2). That shows colour outside the measured band. Keep the bank’s edges or band fraction in the result and use them here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+590
to
+592
| require_choice(self.quantity, "quantity", _OCTAVE_BAND_QUANTITIES) | ||
| require_ranks(self, frequencies=1, values=1) | ||
| require_same_length(self, "frequencies", "values") |
There was a problem hiding this comment.
OctaveBandSpectrum accepts None for its required columns because both guards skip it and OwnsArrays leaves it unchanged. For example, dataclasses.replace(spectrum, frequencies=None) succeeds, but .plot() then fails when _band_axis tries to iterate over None. Call require_present(self, "frequencies", "values") before these guards and cover both fields in the required-field tests.
|
This branch has not been deployed
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.
What changes for you
Four answers that came back as a tuple to be unpacked by position are records with named fields now.
OctaveFilterBank.spectrogramreturns afilters.OctaveSpectrogramResultwithlevels, a read-onlyfrequenciesandtimes; it checks the level grid against both axes and draws itself with.plot(), one raster over time and the logarithmic band centre with a dB colour bar, and a multichannel result asks forchannel=.filters.nominal_frequenciesreturns afilters.FractionalOctaveBandswith read-onlyfrequencies,lower_edgesandupper_edgesand the IEC 61260-1 nominallabelsas a tuple of text; the filter bank reads its band set from the same record, asbank.frequencies,bank.lower_edges,bank.upper_edges,bank.labelsandbank.n_bands, all read-only.building.improvement_octave_bandsand theoctave_bands()methods of the floor-covering, laboratory lining, laboratory floor-covering, joint and rainfall results return abuilding.OctaveBandSpectrumwithfrequencies,valuesin dB and thequantitythey hold, which plots itself in English or Spanish under the symbol its source prints.aircraft.epnl_from_pnltreturns anaircraft.EPNLIntegrationwithepnl,pnltm,bandsharing_adjustment,band_limitsand the derivedduration_correction, andeffective_perceived_noise_levelbuilds itsEPNLResultfrom it.The vibration tolerance limits now come lower first,
(lower, upper), as the filter and weighting masks already did.vibration.weighting_tolerance_percentreturns the ISO 8041-1 Table 5 band lower first, andCENTRAL_TOLERANCE_PERCENT,SKIRT_TOLERANCE_PERCENTandTAIL_TOLERANCE_PERCENThold(lower %, upper %, phase °).vibration.response_tolerance_percentreturns the DIN 45669-1 Table 2 lower limit signed, as the condition of 5.2.3.3 applies it: -10 % from 1,25 f_u to 0,8 f_o, -20 % out to 0,5 f_u and 2 f_o, and -100 %, no lower limit, beyond them.VibrationMeterVerification.lower_percentholds it the same way.roundedis a read-only property wherever a result has one.building.CeilingAttenuationResult.roundedis read frommeasured, to the nearest integer as ASTM E413 clause 5.2 asks (the clause leaves a half open, and the library rounds it up), so it cannot disagree with the data. The cabin, enclosure, screen and measured barrier results exposerounded(androunded_a_weightedon the screen) as properties.The fields that defaulted to
Noneonly so that a hand-built result would still construct are required, and a record built without one, or withNonefor one, raisesTypeErrornaming the field: the measurement chain ofbuilding.AirborneInsulationResultandbuilding.ImpactInsulationResult, the curves of the ISO 717 ratings and their extended forms, the depth of an aperture, the elements of a façade prediction, the probe spacing of an intensity result, the five geometry fields ofenvironment.BarrierInsertionLossand the sound field ofpsychoacoustics.ZwickerLoudness. The reports, plots and geometry drawings lose the branches that refused a record without them.filters.WeightingFiltertakeshigh_accuracyas abool,Trueby default.The errata register records one more misprint: ICAO Annex 16, Volume I (2017), Appendix 2, 4.4.2 prints the bandsharing adjustment as
ΔB = Cavg C(kM), without the minus sign between the two tone corrections. The library implements the difference, which the condition of the same formula and the Nomenclature fix.A tone-corrected perceived noise level is stated in TPNdB, the unit the Nomenclature of ICAO Annex 16, Volume I prints for PNLT and PNLTM. The PNLT series and the PNLTM of
aircraft.EPNLResult,aircraft.EPNLIntegrationandaircraft.RotorcraftEventResultsay so in their documentation and in the notebook table, the.plot()ofEPNLResultlabels its PNLTM point in TPNdB and its axis in PNdB and TPNdB, and the EPNL fiche reads PNLTM [TPNdB]; the PNL stays in PNdB and the EPNL in EPNdB, and no number changes.The EPNL flyover animation (
anim_epnl_flyover) draws the PNLT series and its maximum, so its labels read TPNdB too: its frames change with this layer, and the clip is rendered and published again with it. The microphone label in that clip no longer sits under the ground line, which struck through it.The per-band rows of the three verifiers that return them say
frequencywhere they saidfreq: thebandsrows offilters.FilterComplianceResult,filters.WeightingComplianceResultandemission.IntensityInstrumentComplianceResult, andworst_frequencyin the between-nominals sweep ofWeightingComplianceResult, the spelling every field of the library already uses for the axis.The migration guide has grown past what one fetch of the
llms-start.txtshard can carry, so it travels in a shard of its own,llms-upgrading.txt, listed inllms.txtbeside the start shard.What breaks
None of the new records unpacks: a line that still unpacks one raises
TypeErrorinstead of handing back the wrong column. Read the fields instead:levels, freq, times = bank.spectrogram(x)becomesr = bank.spectrogram(x)withr.levels,r.frequenciesandr.times.freq, lower, upper, labels = filters.nominal_frequencies(3)becomesb = filters.nominal_frequencies(3)withb.frequencies,b.lower_edges,b.upper_edgesandb.labels.bank.freq,bank.freq_d,bank.freq_u,bank.nominal_freqandbank.num_bandsarebank.frequencies,bank.lower_edges,bank.upper_edges,bank.labelsandbank.n_bands.epnl, pnltm, kf, kl = aircraft.epnl_from_pnlt(pnlt)becomesr = aircraft.epnl_from_pnlt(pnlt)withr.epnl,r.pnltmandr.band_limits.freqs, values = building.improvement_octave_bands(dl, f)andresult.octave_bands()becomes.frequenciesands.values.A verifier's band row is read by
band["frequency"], and the between-nominals sweep bybetween_nominals["worst_frequency"];band["freq"]and["worst_freq"]raiseKeyError.The tolerance order is a silent break:
upper, lower = vibration.weighting_tolerance_percent(name, f)still runs and swaps the two arrays, so every unpacking of it and of the three constants has to be read and turned around. A record built by hand needs every required field,CeilingAttenuationResultno longer takesrounded=, andWeightingFilter(..., high_accuracy=None)raisesTypeErrorwhere it used to select the default. The migration guide maps each of these, in English and Spanish, with a section for the records, one for the tolerance order and one for the required fields.How it was verified
The tolerance bands and the sign of the DIN 45669-1 lower limit were read on the printed Table 2 and the 5.2.3.3 condition above it (DIN 45669-1:2010-09, PDF page 19, printed p. 19), with the working range of 5.2.3.1 on the page before. The conformance rows still compare the magnitude Table 2 prints, and
docs/CONFORMANCE.mdis byte-identical. The legend symbols of the octave spectrum follow the prints: ΔL_oct of ISO 10140-1:2021 Formula (H.2), L_Ioct of Formula (K.3), ΔR_oct of ISO 717-1:2020 Formula (D.1), and the plain R_s and L_Inorm where no octave symbol is printed (ISO 10140-1:2021 Figure J.9, PDF page 53, printed p. 47, and Formula (K.5), PDF page 58, printed p. 52). The bandsharing adjustment is read on ICAO Annex 16, Volume I, Eighth Edition, Appendix 2, 4.4.2 (PDF page 158, printed p. APP 2-20) and the Nomenclature entry for ΔB (PDF page 27, printed p. I-7), and the TPNdB of PNLT and PNLTM on the Nomenclature page before it (PDF page 26, printed p. I-6); a new test runseffective_perceived_noise_levelon a flyover whose tone correction is suppressed at PNLTM and checks the adjustment againstC_avg - C(k_M)computed from the result's own tone corrections, and it fails if the adjustment is dropped fromEPNLResult.The full test suite passes on this branch (28943 passed, 69 skipped), and so do the 7560 documentation code blocks over 732 pages, the site build with its type check and HTML validation, the 1893 conformance checks with
docs/CONFORMANCE.mdunchanged, the 77 example fiches, the API reference check, mypy oversrcandscripts, ruff, bandit and the repository'sscripts/check_*.pyguards. In the new spectrogram figure the 250 Hz and 4 kHz tones of the example sit on their own bands, in English, in Spanish and on the dark page, and a content test pins where each band row is drawn. The EPNL figure and the EPNL fiche were redrawn with the TPNdB labels and read in English, in Spanish and on the dark page, and content tests pin the labels of the plot in both languages and the units of the notebook table.The guards
The spelling guard of
tests/test_public_vocabulary.pyreads the public attributes a plain class writes onself, where the filter bank's five abbreviated attributes had stayed out of sight of every field and parameter check, and refusesfreqandfreqsinside any public name. CONTRIBUTING says so in its Words row. Red on the base:tests/test_required_fields.pyreplaces each required field of each of these records withNonethroughdataclasses.replaceand expects aTypeErrornaming the record and the field, and checks thatr_primeandl_nstay optional. Before the records refusedNoneby name, 29 of its 31 cases failed:tests/test_public_vocabulary.pyalso reads the row keys of a verdict of each of the three verifiers that return per-band rows, which no signature or field list shows, and holds them to the same words. Red on the base:One key is exempt, with its reason in the guard:
summation_max_dbofFilterComplianceResult, the upper end of the range of the summation of output signals of IEC 61260-1 5.16 besidesummation_min_db, a bound of a deviation and not the maximum level the max-level rule is about.Green on this branch:
31 passedfor the required fields and the whole vocabulary guard, 94 tests, with the rest of the suite.Left for later, outside these records: a sweep of the remaining tuple-returning functions across the library; the geometry fields of
open_plan,metadiffuser,junction_transmission,panel_transmission,road_absorptionand the noise-control geometry records, which still default toNoneso that a hand-built result can skip them; a name for the two summation keys ofFilterComplianceResultthat says what they bound; and the unit of the bandsharing adjustment, which the Annex 16 Nomenclature also prints in TPNdB and the records still state in dB.