Update unintelligibility algorithm #242
Conversation
Treat any multiple of 60 as a hum frequency Signed-off-by: Dave Thaler <[email protected]>
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant FrequencyInfo
Caller->>FrequencyInfo: IsHumFrequency(frequency)
FrequencyInfo-->>Caller: Boolean result
Note over FrequencyInfo: Check frequency against<br>hum frequency tolerance
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Test/UnintelligibilityTests.cs (2)
40-40: Removeasyncif there are no asynchronous operations.This test method is declared async but does not perform any awaits. Consider removing
asyncto avoid potential confusion, or introduce asynchronous operations if required.
43-52: Add boundary test coverage near tolerance thresholds.Currently, the tolerance is 1.0. You already test frequencies at 59.1 and 60.9, which are close to 60±1.0, but also consider more granular edges like 59.99 or 60.01 to ensure complete coverage of edge cases.
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (1)
128-133: Consider negative or unexpected frequency inputs.This function doesn’t handle negative frequencies or zero, which may be irrelevant in normal usage, but either validate inputs or document assumptions that frequencies will always be positive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs(2 hunks)Test/UnintelligibilityTests.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (2)
101-102: Confirm or adjust the new minimum signal percent value.The comment notes that known-unintelligible peaks at 21% and known-good is at 29%. Setting the default to 25% is in between, but confirm through testing that it doesn’t cause misclassification on borderline cases.
135-135: Good extension of method visibility.Exposing
IsHumFrequency(double frequency)publicly allows for accurate, reusable hum checks across the codebase and in tests.
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (1)
129-136: Consider documenting and parameterizing the frequency tolerance.The implementation looks solid, but a few improvements could make it more maintainable:
- Document why 1.0 Hz was chosen as the tolerance value
- Consider making the tolerance configurable via environment variable, similar to other thresholds in the class
Here's a suggested implementation:
+ // Tolerance in Hz for considering a frequency as a multiple of the hum frequency + const double _defaultHumFrequencyTolerance = 1.0; + private static double HumFrequencyTolerance + { + get + { + string? toleranceString = Environment.GetEnvironmentVariable("ORCASOUND_HUM_FREQUENCY_TOLERANCE"); + return double.TryParse(toleranceString, out var tolerance) ? tolerance : _defaultHumFrequencyTolerance; + } + } private static bool IsHumFrequency(double frequency, double humFrequency) { Debug.Assert(frequency >= 0.0); Debug.Assert(humFrequency >= 0.0); - const double tolerance = 1.0; + double tolerance = HumFrequencyTolerance; double remainder = frequency % humFrequency; return (remainder < tolerance || remainder > (humFrequency - tolerance)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs(3 hunks)Test/UnintelligibilityTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Test/UnintelligibilityTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_windows (Release)
- GitHub Check: build_windows (Debug)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (3)
8-8: LGTM!The addition of the
System.Diagnosticsimport is necessary for theDebug.Assertstatements used in the modifiedIsHumFrequencymethod.
102-103: LGTM! Well-documented threshold adjustment.The new threshold of 25% is well-justified by the documented observations: it provides a balanced separation between known-unintelligible samples (max 21%) and known-good samples (min 29%).
138-138: LGTM! Implementation aligns with PR objectives.The method correctly implements the requirement to treat multiples of both 50 Hz and 60 Hz as hum frequencies, maintaining compatibility with different power line frequencies worldwide.
Treat any multiple of 60 as a hum frequency
Summary by CodeRabbit
New Features
Tests
Improvements