Validate the multiple-shock times with the shared checker - #475
Merged
Conversation
The two guards were written as `not t > 0.0` because that rejects NaN where `t <= 0.0` accepts it, with the reason in a comment beside each one. The library already has `require_positive` for exactly this, written the same way for the same reason: it rejects NaN and infinities, and it names the parameter that failed rather than both at once. Using it removes the last hand-written positivity check in the module, drops the marker that told the analyzer to look away, and makes the tests assert the message the rest of the library gives. Infinity is now rejected where it was accepted before, which is the fix the shared checker brings for free.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
github-actions
Bot
added
documentation
|
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR replaces hand-written time positivity guards in the human multiple-shock vibration routines with the shared require_positive helper, and aligns tests and changelog to reflect the new validation behavior and error messages (including rejection of infinity). Sequence diagram for daily_dose validation with require_positivesequenceDiagram
actor Client
participant daily_dose
participant require_positive
Client->>daily_dose: daily_dose(dose, exposure_time, measurement_time)
daily_dose->>require_positive: require_positive(exposure_time, exposure_time)
require_positive-->>daily_dose: exposure_time
daily_dose->>require_positive: require_positive(measurement_time, measurement_time)
require_positive-->>daily_dose: measurement_time
daily_dose-->>Client: float(dose * (exposure_time / measurement_time) ** (1.0 / DOSE_EXPONENT))
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMultiple-shock time parameters now use ChangesMultiple-shock validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR standardizes the validation of multiple-shock time-like parameters by routing them through the shared require_positive validator, aligning behavior and error messages with the rest of the library and making NaN/∞ handling explicit and test-covered.
Changes:
- Replace hand-rolled time guards in
vibration.human.multiple_shockwithrequire_positiveforexposure_time,measurement_time, anddays_per_year. - Extend tests to assert rejection of both NaN and infinity with parameter-specific error messages.
- Document the behavior change (including infinity now being rejected) in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/vibration/human/test_multiple_shock_vibration.py | Updates tests to cover NaN and infinity rejection with per-parameter error messages. |
| src/phonometry/vibration/human/multiple_shock.py | Switches multiple-shock time validation to the shared require_positive helper for consistency. |
| CHANGELOG.md | Notes the validation change and the resulting NaN/∞ semantics and improved error messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #475 +/- ##
==========================================
- Coverage 96.99% 96.99% -0.01%
==========================================
Files 258 258
Lines 38520 38519 -1
==========================================
- Hits 37363 37362 -1
Misses 1157 1157 ☔ 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 — 47586 tests, 0 failures (✅ all green)
Conformance harness: |
|
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
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 and why
The two time guards in
vibration.human.multiple_shockwere written asnot t > 0.0, which rejects NaN wheret <= 0.0accepts it. That is correct and it is why they were written that way, but the reason lived in a comment and the analyzer read them as an inverted comparison.The library already has
require_positivefor exactly this, written the same way for the same reason. Using it removes the last hand-written positivity check in the module, drops the marker that told the analyzer to look away, and gives the caller the message the rest of the library gives, naming the parameter that failed rather than both at once.Infinity is rejected now where it was accepted before, which is what the shared checker brings for free. The test covers both NaN and infinity through all three parameters.
Validation
No new computation. The full vibration suite passes and the conformance report is unchanged.
Checklist
ruff check .mypy src scriptspytest -q(vibration suite; full suite in CI)make conformance(no diff)[Unreleased]Summary by Sourcery
Route multiple-shock time parameter validation through the shared positivity checker for more consistent error handling and robustness.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit