Update unintelligibility algorithm #250
Conversation
Signed-off-by: Dave Thaler <[email protected]>
WalkthroughThe pull request introduces significant changes to the audio signal intelligibility analysis in the OrcanodeMonitor system. The modifications focus on shifting from using maximum amplitude to calculating total magnitudes across different frequency ranges. New methods have been added to the Changes
Assessment against linked issues
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: 1
🧹 Nitpick comments (4)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (2)
162-204: Consider refactoring to reduce code duplication.The
GetTotalNonHumMagnitudeandGetTotalHumMagnitudemethods share similar logic. Consider extracting the common pattern into a private helper method.+private double GetTotalMagnitudeForFrequencies(Func<double, bool> frequencyPredicate) +{ + return FrequencyMagnitudes + .Where(pair => frequencyPredicate(pair.Key) && pair.Value > MinNoiseMagnitude) + .Sum(pair => pair.Value); +} public double GetTotalNonHumMagnitude() -{ - double totalNonHumMagnitude = 0; - foreach (var pair in FrequencyMagnitudes) - { - double frequency = pair.Key; - double magnitude = pair.Value; - if (!IsHumFrequency(frequency)) - { - if (magnitude > MinNoiseMagnitude) - { - totalNonHumMagnitude += magnitude; - } - } - } - return totalNonHumMagnitude; -} +{ + return GetTotalMagnitudeForFrequencies(frequency => !IsHumFrequency(frequency)); +} public double GetTotalHumMagnitude() -{ - double totalHumMagnitude = 0; - foreach (var pair in FrequencyMagnitudes) - { - double frequency = pair.Key; - double magnitude = pair.Value; - if (IsHumFrequency(frequency)) - { - if (magnitude > MinNoiseMagnitude) - { - totalHumMagnitude += magnitude; - } - } - } - return totalHumMagnitude; -} +{ + return GetTotalMagnitudeForFrequencies(IsHumFrequency); +}
221-226: Improve variable naming for clarity.The variable
maxNonHumMagnitudeis used alongside total magnitudes but isn't used in the ratio calculation, which might be confusing.-double maxNonHumMagnitude = GetMaxNonHumMagnitude(); -double totalNonHumMagnitude = GetTotalNonHumMagnitude(); -double totalHumMagnitude = GetTotalHumMagnitude(); - -if (maxNonHumMagnitude < MinNoiseMagnitude || totalNonHumMagnitude / totalHumMagnitude < MinSignalRatio) +// Calculate magnitudes for signal analysis +double peakNonHumMagnitude = GetMaxNonHumMagnitude(); +double totalSignalMagnitude = GetTotalNonHumMagnitude(); +double totalHumMagnitude = GetTotalHumMagnitude(); + +if (peakNonHumMagnitude < MinNoiseMagnitude || totalSignalMagnitude / totalHumMagnitude < MinSignalRatio)Test/UnintelligibilityTests.cs (1)
74-74: Consider adding test cases for total magnitude calculations.While the new test case is good, consider adding specific tests to verify the total magnitude calculations and ratio-based intelligibility determination.
Example test cases to consider:
[TestMethod] public async Task TestTotalMagnitudeRatios() { // Test borderline unintelligible case (ratio just below threshold) await TestSampleAsync("unintelligible\\live4869.ts", OrcanodeOnlineStatus.Unintelligible); // Test borderline intelligible case (ratio just above threshold) await TestSampleAsync("normal\\live4118.ts", OrcanodeOnlineStatus.Online); }docs/Design.md (1)
116-116: Add migration guidance for existing deployments.This is a breaking change in how audio intelligibility is measured. Consider adding:
- Migration steps for existing deployments
- Instructions for recalibrating hydrophone configurations
- Expected impact on existing monitoring systems
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs(3 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml(1 hunks)OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs(2 hunks)Test/UnintelligibilityTests.cs(1 hunks)docs/Design.md(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 (4)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (1)
102-103: LGTM! Clear documentation of threshold rationale.The comment effectively explains the empirical basis for the threshold values.
OrcanodeMonitor/Pages/SpectralDensity.cshtml.cs (1)
29-32: LGTM! Consistent with core algorithm changes.The new properties and SignalRatio calculation align well with the updated unintelligibility algorithm.
Also applies to: 89-91
OrcanodeMonitor/Pages/SpectralDensity.cshtml (1)
78-80: LGTM! Enhanced visibility of signal analysis.The added statistics provide valuable insight into the total magnitudes used for intelligibility determination.
docs/Design.md (1)
116-116: Verify the new default value of 150%.The change from 30% to 150% is substantial. Please ensure this value has been thoroughly tested across different hydrophone deployments and environmental conditions.
Run the following script to analyze test coverage and historical data:
✅ Verification successful
The default value of 150% is empirically justified
The codebase shows this value was carefully chosen based on empirical measurements:
- Known unintelligible samples max out at 53%
- Known good samples start from 208%
- The 150% threshold provides a clear separation between these ranges
The value is well-tested through comprehensive test cases covering unintelligible, normal, and borderline scenarios, with support for runtime configuration via environment variables if needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage and historical data for intelligibility detection # Check for test files related to intelligibility echo "Checking test coverage:" rg -l "intelligib" --type-add 'test:*.{cs,py}' -t test # Look for historical configuration values in git history echo -e "\nChecking historical configuration values:" git log -p -- . | rg "MIN_INTELLIGIBLE_SIGNAL_PERCENT.*Default:"Length of output: 359
Script:
#!/bin/bash # Examine test and implementation files for threshold validation and usage echo "=== Test File Content ===" cat Test/UnintelligibilityTests.cs echo -e "\n=== Implementation Files ===" echo "--- FfmpegCoreAnalyzer.cs ---" cat OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs echo -e "\n=== Searching for config files ===" fd -e json -e yaml -e yml -e config | xargs rg -l "MIN_INTELLIGIBLE_SIGNAL_PERCENT"Length of output: 17893
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs (1)
162-204: Consider refactoring to reduce code duplication.The new methods are well-implemented and documented. However, they share similar logic patterns that could be consolidated.
Consider refactoring both methods to use a common helper method:
+ private double GetTotalMagnitudeForFrequencies(Func<double, bool> frequencyPredicate) + { + return FrequencyMagnitudes + .Where(pair => frequencyPredicate(pair.Key) && pair.Value > MinNoiseMagnitude) + .Sum(pair => pair.Value); + } public double GetTotalNonHumMagnitude() { - double totalNonHumMagnitude = 0; - foreach (var pair in FrequencyMagnitudes) - { - double frequency = pair.Key; - double magnitude = pair.Value; - if (!IsHumFrequency(frequency)) - { - if (magnitude > MinNoiseMagnitude) - { - totalNonHumMagnitude += magnitude; - } - } - } - return totalNonHumMagnitude; + return GetTotalMagnitudeForFrequencies(frequency => !IsHumFrequency(frequency)); } public double GetTotalHumMagnitude() { - double totalHumMagnitude = 0; - foreach (var pair in FrequencyMagnitudes) - { - double frequency = pair.Key; - double magnitude = pair.Value; - if (IsHumFrequency(frequency)) - { - if (magnitude > MinNoiseMagnitude) - { - totalHumMagnitude += magnitude; - } - } - } - return totalHumMagnitude; + return GetTotalMagnitudeForFrequencies(IsHumFrequency); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
OrcanodeMonitor/Core/FfmpegCoreAnalyzer.cs(3 hunks)docs/Design.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/Design.md
⏰ 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)
102-103: Well-documented threshold adjustment based on empirical data!The new threshold of 150% is well-justified with empirical observations showing clear separation between known-unintelligible (max 53%) and known-good samples (min 208%).
221-230: Verify the impact of switching from max to total magnitude calculations.Since this is a significant change in how signal intelligibility is determined, we should verify that all dependent code is updated accordingly.
Let's check for any direct usage of the old method:
✅ Verification successful
✓ Changes to magnitude calculations are properly implemented
The switch to using total magnitudes for the signal ratio calculation is well-structured:
- Initial signal detection still uses max magnitude as a quick check
- Signal ratio uses total magnitudes for better accuracy
- UI updated to show both metrics
- No conflicts found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of GetMaxNonHumMagnitude rg "GetMaxNonHumMagnitude" --type cs # Search for any references to "magnitude" in test files rg "magnitude" --type cs -g "*test*.cs"Length of output: 545
Script:
#!/bin/bash # Find test files fd "test" -e cs # Search for all magnitude-related methods ast-grep --pattern 'public $_ $$$Magnitude$$$' # Search for any magnitude calculations rg "magnitude" -i --type cs -A 3Length of output: 17421
| // Find the total magnitude outside the audio hum range. | ||
| if (GetMaxNonHumMagnitude() < MinNoiseMagnitude) | ||
| { | ||
| // Just silence outside the hum range, no signal. | ||
| return OrcanodeOnlineStatus.Unintelligible; | ||
| } | ||
|
|
||
| if (maxNonHumMagnitude < MinNoiseMagnitude || maxNonHumMagnitude / max < MinSignalRatio) | ||
| double totalNonHumMagnitude = GetTotalNonHumMagnitude(); | ||
| double totalHumMagnitude = GetTotalHumMagnitude(); | ||
| if (totalNonHumMagnitude / totalHumMagnitude < MinSignalRatio) |
There was a problem hiding this comment.
Fix inconsistent use of max vs. total magnitudes in status determination.
The current implementation mixes two approaches:
- First checks max non-hum magnitude (line 222)
- Then switches to using total magnitudes (lines 228-230)
This inconsistency could lead to confusing behavior.
Consider using total magnitudes consistently:
- // Find the total magnitude outside the audio hum range.
- if (GetMaxNonHumMagnitude() < MinNoiseMagnitude)
+ // Check if there's any significant non-hum signal
+ double totalNonHumMagnitude = GetTotalNonHumMagnitude();
+ if (totalNonHumMagnitude == 0)
{
// Just silence outside the hum range, no signal.
return OrcanodeOnlineStatus.Unintelligible;
}
- double totalNonHumMagnitude = GetTotalNonHumMagnitude();
double totalHumMagnitude = GetTotalHumMagnitude();
if (totalNonHumMagnitude / totalHumMagnitude < MinSignalRatio)Committable suggestion skipped: line range outside the PR's diff.
Fixes #249
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Tests