feat: reduce false positive detections#2038
feat: reduce false positive detections#2038jeremyames-gmail wants to merge 1 commit intotphakala:mainfrom
Conversation
Summary of ChangesHello @jeremyames-gmail, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the accuracy and reliability of bird detection by significantly reducing false positives. It achieves this by adjusting several default threshold values, refining the logic for dynamic threshold escalation, and correcting a specific bug related to species-level zero thresholds. These changes are applied to default configurations, aiming to improve out-of-the-box performance for new installations without altering existing user settings. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughBumps defaults (range filter 0.01→0.03, dynamic threshold min 0.20→0.40), changes dynamic-threshold learning to multi-level progression with detection-count gates, treats per-species threshold 0 as fallback to global, and updates tests and docs accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
|
/gemini review |
coderabbitai
Bot
added
bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several changes to reduce false positive detections, including raising the minimum dynamic threshold and range filter threshold, and slowing down the dynamic threshold escalation. It also includes a bug fix for species-specific thresholds set to zero and enables the false positive filter by default. The changes are well-supported by updated tests and documentation. My review identifies a minor improvement opportunity to replace magic numbers with constants for better code clarity and maintainability.
| switch { | ||
| case dt.HighConfCount >= 6: | ||
| // Level 3: maximum reduction (75% off base) — requires 6+ approved detections | ||
| dt.Level = 3 | ||
| dt.CurrentValue = float64(baseThreshold * thresholdLevel3Multiplier) | ||
| case dt.HighConfCount >= 4: | ||
| // Level 2: 50% reduction — requires 4+ approved detections | ||
| dt.Level = 2 | ||
| dt.CurrentValue = float64(baseThreshold * thresholdLevel2Multiplier) | ||
| case dt.HighConfCount >= 2: | ||
| // Level 1: 25% reduction — requires 2+ approved detections | ||
| dt.Level = 1 | ||
| dt.CurrentValue = float64(baseThreshold * thresholdLevel1Multiplier) | ||
| default: | ||
| // Level 3 is the maximum reduction; any count >= 3 stays at this level | ||
| dt.Level = 3 | ||
| dt.CurrentValue = float64(baseThreshold * thresholdLevel3Multiplier) | ||
| // Not enough evidence yet — stay at base threshold | ||
| dt.CurrentValue = float64(baseThreshold) | ||
| } |
There was a problem hiding this comment.
The detection counts 6, 4, and 2 for triggering different threshold levels are magic numbers. To improve readability and maintainability, consider defining them as named constants at the package level. For example:
const (
detectionsForLevel1 = 2
detectionsForLevel2 = 4
detectionsForLevel3 = 6
)Then, these constants can be used in the switch statement.
| // Trigger Level 3 (25% of 0.80 = 0.20, which is below min of 0.30) | ||
| for i := range 3 { | ||
| // Now requires 6 approved detections for Level 3 | ||
| for i := range 6 { |
There was a problem hiding this comment.
This test uses the magic number 6. If you define constants for the detection counts as suggested in dynamic_threshold.go (e.g., detectionsForLevel3), please use that constant here as well to keep the test synchronized with the implementation and improve clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
doc/wiki/guide.md (2)
857-908:⚠️ Potential issue | 🟡 MinorDynamic threshold docs still mention the old 0.20 minimum.
The config snippet now shows
min: 0.40, but the “Key Parameters” explanation and the practical example later in this section still cite 0.20 / 0.2. Please update those to 0.40 for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/wiki/guide.md` around lines 857 - 908, Update the docs to reflect the new default minimum of 0.40: change the "min" description under "Key Parameters Explained" (currently showing 0.20) to 0.40, update the Threshold Adjustment Levels table's example for level 3+ from 0.20 (min limit) to 0.40 (min limit), and change the Practical Example block where it lists min: 0.2 to min: 0.4 so that all references to the `min` parameter and examples match the config snippet's `min: 0.40`.
770-781:⚠️ Potential issue | 🟡 MinorUpdate remaining references to the old 0.01 default.
These edits set the range filter default to 0.03, but other sections still describe 0.01 as the default (e.g., earlier config examples and the Stage 1 narrative). Please align all references to avoid conflicting guidance.
Also applies to: 1149-1176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/wiki/guide.md` around lines 770 - 781, Update all remaining references to the old 0.01 default so the docs consistently state 0.03 for the range filter; search for occurrences of "0.01", "range filter default", and the config key birdnet.rangefilter.threshold (including the earlier config examples and the Stage 1 narrative noted around the other section) and change them to 0.03, adjusting YAML examples, prose, and any explanatory text so the default value and its explanation match the new 0.03 setting.internal/conf/defaults.go (1)
76-170: 🛠️ Refactor suggestion | 🟠 MajorReplace new literal defaults with named constants.
The new 0.03 / 0.40 / 2 values are magic numbers in Go code. Please define descriptive constants and reference them in
SetDefaultcalls to align with project standards.♻️ Suggested refactor
+const ( + defaultRangeFilterThreshold = 0.03 + defaultDynamicThresholdMin = 0.40 + defaultFalsePositiveFilterLvl = 2 +) ... - viper.SetDefault("birdnet.rangefilter.threshold", 0.03) + viper.SetDefault("birdnet.rangefilter.threshold", defaultRangeFilterThreshold) ... - viper.SetDefault("realtime.dynamicthreshold.min", 0.40) + viper.SetDefault("realtime.dynamicthreshold.min", defaultDynamicThresholdMin) ... - viper.SetDefault("realtime.falsepositivefilter.level", 2) + viper.SetDefault("realtime.falsepositivefilter.level", defaultFalsePositiveFilterLvl)As per coding guidelines, "No magic numbers or strings - use named constants with descriptive names in Go code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/conf/defaults.go` around lines 76 - 170, Replace the magic literals by defining descriptive package-level constants (e.g. const DefaultRangeFilterThreshold = 0.03, DefaultDynamicThresholdMin = 0.40, DefaultFalsePositiveFilterLevel = 2) in the same defaults.go (or package scope) and use those constants in the viper.SetDefault calls for "birdnet.rangefilter.threshold", "realtime.dynamicthreshold.min" and "realtime.falsepositivefilter.level" respectively; ensure the constant names are clear and referenced in place of the numeric literals so the functions/keys BirdNet range filter threshold, DynamicThreshold.min, and FalsePositiveFilter.level use the named constants.internal/analysis/processor/dynamic_threshold_test.go (2)
241-242:⚠️ Potential issue | 🟡 MinorStale comment:
// Higher minimumis now incorrect.
newTestProcessor()was updated in this PR to setMin: 0.40; the test overrides it to0.30, which is lower than the new default. The comment "Higher minimum" no longer applies.✏️ Proposed fix
- p.Settings.Realtime.DynamicThreshold.Min = 0.30 // Higher minimum + p.Settings.Realtime.DynamicThreshold.Min = 0.30 // Lower custom minimum (Level 3 computes 0.20 < 0.30, so value clamps to 0.30)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/dynamic_threshold_test.go` around lines 241 - 242, The inline comment next to the test override is now incorrect: update the comment on the line that sets p.Settings.Realtime.DynamicThreshold.Min in the test using newTestProcessor() to reflect that the override sets a lower value than the new default (newTestProcessor now sets Min to 0.40), e.g. replace "// Higher minimum" with a correct note such as "// Lower minimum than default (default 0.40)" or remove the comment entirely; target the newTestProcessor usage and the p.Settings.Realtime.DynamicThreshold.Min assignment when making the change.
263-273:⚠️ Potential issue | 🟡 MinorUse
require.NotNilto prevent nil-pointer panic on test failure.
assert.NotNilon line 270 does not stop the test on failure, so lines 271–273 will dereference a nil*DynamicThresholdand panic instead of producing a clean failure message ifLearnFromApprovedDetectiondoesn't create the entry.🛡️ Proposed fix
-import "github.com/stretchr/testify/assert" +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +)- assert.NotNil(t, p.DynamicThresholds["new species"], "Should create threshold entry") + require.NotNil(t, p.DynamicThresholds["new species"], "Should create threshold entry") assert.Equal(t, 0, p.DynamicThresholds["new species"].Level, "Level should be 0 (need 2 for Level 1)") assert.Equal(t, 1, p.DynamicThresholds["new species"].HighConfCount, "HighConfCount should be 1") assert.Equal(t, "Newus speciesus", p.DynamicThresholds["new species"].ScientificName, "ScientificName should be set")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/dynamic_threshold_test.go` around lines 263 - 273, Replace the non-fatal assertion with a fatal one in TestLearnFromApprovedDetectionInitializesIfMissing: change assert.NotNil(t, p.DynamicThresholds["new species"], ...) to require.NotNil(t, p.DynamicThresholds["new species"], ...), and add the require package import ("github.com/stretchr/testify/require") at the top of the test file so the test stops immediately and avoids nil-pointer dereference when p.DynamicThresholds["new species"] is missing.
🧹 Nitpick comments (1)
internal/analysis/processor/dynamic_threshold_test.go (1)
44-55: Consider addingt.Parallel()to independent test functions.Every test here creates its own
Processorinstance with no shared state, making them safe to run in parallel. Addingt.Parallel()to each top-level test function would reduce total test-suite wall time.♻️ Example (apply to all top-level `TestXxx` functions)
func TestCustomThresholdRespected(t *testing.T) { + t.Parallel() p := newTestProcessor()As per coding guidelines, "Use
t.Parallel()for top-level test functions where safe" and "Uset.Parallel()only for independent tests that don't mutate global state."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/dynamic_threshold_test.go` around lines 44 - 55, This test is independent and can run concurrently—add t.Parallel() at the start of the top-level test function TestCustomThresholdRespected so it runs in parallel with other independent tests; locate the TestCustomThresholdRespected function (which constructs a Processor via newTestProcessor and calls p.getAdjustedConfidenceThreshold) and insert t.Parallel() as the first statement in the function body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/analysis/processor/base_threshold_test.go`:
- Around line 10-57: The three nearly-identical tests
(TestGetBaseConfidenceThreshold_ZeroThresholdFallsThrough,
TestGetBaseConfidenceThreshold_ExplicitThresholdUsed,
TestGetBaseConfidenceThreshold_NotInConfigUsesGlobal) should be collapsed into a
single table-driven test that iterates a slice of cases and uses t.Run for each
case; for each case create a fresh processor via newTestProcessor(), set
p.Settings.BirdNET.Threshold and p.Settings.Realtime.Species per the case, call
p.getBaseConfidenceThreshold(...) and assert.InDelta against the expected value,
and call t.Parallel() inside each subtest if running them concurrently is safe.
In `@internal/analysis/processor/dynamic_threshold.go`:
- Around line 206-224: The code uses magic literals 2, 4, and 6 to decide
progression levels; define descriptive constants (e.g., level1MinDetections = 2,
level2MinDetections = 4, level3MinDetections = 6) and replace the numeric
literals in the switch condition (dt.HighConfCount >= ...) with those constants,
update the comments on the cases to reference the named constants and the
intended cadence, and keep existing assignments to dt.Level and dt.CurrentValue
(which use baseThreshold and thresholdLevel1Multiplier/2/3) unchanged so
behavior remains identical.
---
Outside diff comments:
In `@doc/wiki/guide.md`:
- Around line 857-908: Update the docs to reflect the new default minimum of
0.40: change the "min" description under "Key Parameters Explained" (currently
showing 0.20) to 0.40, update the Threshold Adjustment Levels table's example
for level 3+ from 0.20 (min limit) to 0.40 (min limit), and change the Practical
Example block where it lists min: 0.2 to min: 0.4 so that all references to the
`min` parameter and examples match the config snippet's `min: 0.40`.
- Around line 770-781: Update all remaining references to the old 0.01 default
so the docs consistently state 0.03 for the range filter; search for occurrences
of "0.01", "range filter default", and the config key
birdnet.rangefilter.threshold (including the earlier config examples and the
Stage 1 narrative noted around the other section) and change them to 0.03,
adjusting YAML examples, prose, and any explanatory text so the default value
and its explanation match the new 0.03 setting.
In `@internal/analysis/processor/dynamic_threshold_test.go`:
- Around line 241-242: The inline comment next to the test override is now
incorrect: update the comment on the line that sets
p.Settings.Realtime.DynamicThreshold.Min in the test using newTestProcessor() to
reflect that the override sets a lower value than the new default
(newTestProcessor now sets Min to 0.40), e.g. replace "// Higher minimum" with a
correct note such as "// Lower minimum than default (default 0.40)" or remove
the comment entirely; target the newTestProcessor usage and the
p.Settings.Realtime.DynamicThreshold.Min assignment when making the change.
- Around line 263-273: Replace the non-fatal assertion with a fatal one in
TestLearnFromApprovedDetectionInitializesIfMissing: change assert.NotNil(t,
p.DynamicThresholds["new species"], ...) to require.NotNil(t,
p.DynamicThresholds["new species"], ...), and add the require package import
("github.com/stretchr/testify/require") at the top of the test file so the test
stops immediately and avoids nil-pointer dereference when
p.DynamicThresholds["new species"] is missing.
In `@internal/conf/defaults.go`:
- Around line 76-170: Replace the magic literals by defining descriptive
package-level constants (e.g. const DefaultRangeFilterThreshold = 0.03,
DefaultDynamicThresholdMin = 0.40, DefaultFalsePositiveFilterLevel = 2) in the
same defaults.go (or package scope) and use those constants in the
viper.SetDefault calls for "birdnet.rangefilter.threshold",
"realtime.dynamicthreshold.min" and "realtime.falsepositivefilter.level"
respectively; ensure the constant names are clear and referenced in place of the
numeric literals so the functions/keys BirdNet range filter threshold,
DynamicThreshold.min, and FalsePositiveFilter.level use the named constants.
---
Nitpick comments:
In `@internal/analysis/processor/dynamic_threshold_test.go`:
- Around line 44-55: This test is independent and can run concurrently—add
t.Parallel() at the start of the top-level test function
TestCustomThresholdRespected so it runs in parallel with other independent
tests; locate the TestCustomThresholdRespected function (which constructs a
Processor via newTestProcessor and calls p.getAdjustedConfidenceThreshold) and
insert t.Parallel() as the first statement in the function body.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
doc/wiki/guide.mdinternal/analysis/processor/base_threshold_test.gointernal/analysis/processor/dynamic_threshold.gointernal/analysis/processor/dynamic_threshold_test.gointernal/analysis/processor/processor.gointernal/conf/config.yamlinternal/conf/defaults.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/analysis/processor/dynamic_threshold.go`:
- Around line 214-229: The switch assigns dt.Level using numeric literals
(1/2/3); define descriptive constants (e.g., const LevelBase = 0, LevelOne = 1,
LevelTwo = 2, LevelThree = 3) near the top of dynamic_threshold.go (or alongside
existing threshold constants) and replace the numeric assignments in the switch
(dt.Level = 3 / 2 / 1) with those named constants (dt.Level = LevelThree /
LevelTwo / LevelOne) so there are no magic numbers in dt.Level usage.
There was a problem hiding this comment.
Code Review
This pull request implements several measures to reduce false positive detections, including raising default thresholds, slowing dynamic threshold escalation, and fixing a bug in species-specific threshold fallbacks. No security vulnerabilities were found within the scope of the reviewed changes, with database operations using proper parameterization and no hardcoded secrets identified. The changes are well-documented, accompanied by comprehensive unit tests, and the code adheres to established patterns, correctly handling edge cases related to Go's zero-value unmarshaling.
| case dt.HighConfCount >= thresholdLevel1MinDetections: | ||
| dt.Level = 1 |
There was a problem hiding this comment.
In the default case of the switch statement, dt.Level should be explicitly set to 0 to ensure state consistency. While dt.Level is initialized to 0, being explicit here prevents potential issues if the state was modified elsewhere or if HighConfCount was reset without a corresponding Level reset. This aligns with the principle of ensuring all fields are set to a valid default state, even if some are re-initialized later, which is crucial for maintaining predictable behavior, especially in testing scenarios.
default:
// Not enough evidence yet — stay at base threshold
dt.Level = 0
dt.CurrentValue = float64(baseThreshold)References
- When a struct is initialized in a constructor, ensure all fields are set to a valid default state, even if some are re-initialized later. This is crucial for tests that may use the constructor directly without the full application initialization.
| if config, exists := lookupSpeciesConfig(p.Settings.Realtime.Species.Config, commonName, scientificName); exists { | ||
| // Check if species has a custom threshold using both common and scientific name lookup. | ||
| // Species with threshold: 0 (actions-only config) fall through to global threshold. | ||
| if config, exists := lookupSpeciesConfig(p.Settings.Realtime.Species.Config, commonName, scientificName); exists && config.Threshold > 0 { |
There was a problem hiding this comment.
The addition of config.Threshold > 0 correctly addresses the bug where species with an unconfigured (zero) threshold would accept all detections. This ensures they correctly fall through to the global threshold as intended for 'actions-only' configurations.
jeremyames-gmail
force-pushed
the
feat/false-positive-reduction
branch
from
b5ec3f9 to
37f2e4c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/analysis/processor/base_threshold_test.go (1)
10-57: Consolidate these three tests into a single table-driven suite.Same setup and only the inputs/expected values change; a case table keeps the tests easier to maintain.
As per coding guidelines, “Write table-driven tests for all packages”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/base_threshold_test.go` around lines 10 - 57, Consolidate the three tests into one table-driven test that iterates cases and calls p.getBaseConfidenceThreshold; create a test function (e.g., TestGetBaseConfidenceThreshold_Table) that for each case (name string) sets up p := newTestProcessor(), p.Settings.BirdNET.Threshold = 0.75 and p.Settings.Realtime.Species = conf.SpeciesSettings{Config: ...} as needed per case, define fields like inputCommonName, inputScientificName, and expectedThreshold, then loop over cases with t.Run(case.name, func(t *testing.T){ threshold := p.getBaseConfidenceThreshold(case.inputCommonName, case.inputScientificName); assert.InDelta(t, case.expectedThreshold, float64(threshold), 0.001) }); keep unique symbols (newTestProcessor, p.getBaseConfidenceThreshold, p.Settings.BirdNET.Threshold, p.Settings.Realtime.Species, conf.SpeciesSettings) to locate and replace the existing three tests.
🧹 Nitpick comments (2)
internal/analysis/processor/dynamic_threshold_test.go (1)
136-175: Use named constants for the 2/4/6 detection tiers and base thresholds in this test.These values represent core behavior; extracting them to constants (or reusing production constants if available) avoids future drift.
As per coding guidelines, “No magic numbers or strings - use named constants with descriptive names in Go code”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/dynamic_threshold_test.go` around lines 136 - 175, The test TestLearnFromApprovedDetectionLevels uses magic numbers for the base threshold (0.80), the learning cooldown spacing (15s), the tier counts (2, 4, 6) and the minimum clamp (0.40); replace those literals with named constants (e.g., baseThreshold := baseDynamicThreshold, learningCooldown := dynamicThresholdLearningCooldown, tierLevel1Count := dynamicTierLevel1Count, tierLevel2Count := dynamicTierLevel2Count, tierLevel3Count := dynamicTierLevel3Count, minThreshold := dynamicThresholdMin) and use those constants when calling p.addSpeciesToDynamicThresholds, setting LastLearnedAt spacing, and asserting p.DynamicThresholds["test species"].CurrentValue and Level so the test mirrors production constants and eliminates magic numbers.internal/analysis/processor/base_threshold_test.go (1)
13-55: Extract repeated threshold and delta literals into named constants.This removes magic numbers and makes intent clearer across the three tests.
♻️ Example refactor
+const ( + globalThreshold = 0.75 + explicitThreshold = 0.90 + delta = 0.001 +) + func TestGetBaseConfidenceThreshold_ZeroThresholdFallsThrough(t *testing.T) { p := newTestProcessor() - p.Settings.BirdNET.Threshold = 0.75 // global threshold + p.Settings.BirdNET.Threshold = globalThreshold // global threshold ... - assert.InDelta(t, 0.75, float64(threshold), 0.001, + assert.InDelta(t, globalThreshold, float64(threshold), delta, "Species with threshold:0 should fall through to global threshold, not return 0.0") } ... - assert.InDelta(t, 0.90, float64(threshold), 0.001, + assert.InDelta(t, explicitThreshold, float64(threshold), delta, "Species with explicit threshold should use that value")As per coding guidelines, “No magic numbers or strings - use named constants with descriptive names in Go code”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/base_threshold_test.go` around lines 13 - 55, Replace the magic numeric literals used across the tests with named constants: declare descriptive constants (e.g., globalThreshold = 0.75, explicitThreshold = 0.90, actionsOnlyThreshold = 0.0, and thresholdDelta = 0.001) at the top of the test file and update TestGetBaseConfidenceThreshold_ActionsOnlyFallsThrough, TestGetBaseConfidenceThreshold_ExplicitThresholdUsed, and TestGetBaseConfidenceThreshold_NotInConfigUsesGlobal to use those constants when setting p.Settings.BirdNET.Threshold, the species config Threshold values, and the assert.InDelta expected/delta parameters; keep calls to p.getBaseConfidenceThreshold unchanged. Ensure constant names are clear and used in all three tests to remove the magic numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/analysis/processor/base_threshold_test.go`:
- Around line 10-57: Consolidate the three tests into one table-driven test that
iterates cases and calls p.getBaseConfidenceThreshold; create a test function
(e.g., TestGetBaseConfidenceThreshold_Table) that for each case (name string)
sets up p := newTestProcessor(), p.Settings.BirdNET.Threshold = 0.75 and
p.Settings.Realtime.Species = conf.SpeciesSettings{Config: ...} as needed per
case, define fields like inputCommonName, inputScientificName, and
expectedThreshold, then loop over cases with t.Run(case.name, func(t
*testing.T){ threshold := p.getBaseConfidenceThreshold(case.inputCommonName,
case.inputScientificName); assert.InDelta(t, case.expectedThreshold,
float64(threshold), 0.001) }); keep unique symbols (newTestProcessor,
p.getBaseConfidenceThreshold, p.Settings.BirdNET.Threshold,
p.Settings.Realtime.Species, conf.SpeciesSettings) to locate and replace the
existing three tests.
---
Nitpick comments:
In `@internal/analysis/processor/base_threshold_test.go`:
- Around line 13-55: Replace the magic numeric literals used across the tests
with named constants: declare descriptive constants (e.g., globalThreshold =
0.75, explicitThreshold = 0.90, actionsOnlyThreshold = 0.0, and thresholdDelta =
0.001) at the top of the test file and update
TestGetBaseConfidenceThreshold_ActionsOnlyFallsThrough,
TestGetBaseConfidenceThreshold_ExplicitThresholdUsed, and
TestGetBaseConfidenceThreshold_NotInConfigUsesGlobal to use those constants when
setting p.Settings.BirdNET.Threshold, the species config Threshold values, and
the assert.InDelta expected/delta parameters; keep calls to
p.getBaseConfidenceThreshold unchanged. Ensure constant names are clear and used
in all three tests to remove the magic numbers.
In `@internal/analysis/processor/dynamic_threshold_test.go`:
- Around line 136-175: The test TestLearnFromApprovedDetectionLevels uses magic
numbers for the base threshold (0.80), the learning cooldown spacing (15s), the
tier counts (2, 4, 6) and the minimum clamp (0.40); replace those literals with
named constants (e.g., baseThreshold := baseDynamicThreshold, learningCooldown
:= dynamicThresholdLearningCooldown, tierLevel1Count := dynamicTierLevel1Count,
tierLevel2Count := dynamicTierLevel2Count, tierLevel3Count :=
dynamicTierLevel3Count, minThreshold := dynamicThresholdMin) and use those
constants when calling p.addSpeciesToDynamicThresholds, setting LastLearnedAt
spacing, and asserting p.DynamicThresholds["test species"].CurrentValue and
Level so the test mirrors production constants and eliminates magic numbers.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
doc/wiki/guide.mdinternal/analysis/processor/base_threshold_test.gointernal/analysis/processor/dynamic_threshold.gointernal/analysis/processor/dynamic_threshold_test.gointernal/analysis/processor/processor.gointernal/conf/config.yamlinternal/conf/defaults.go
🚧 Files skipped from review as they are similar to previous changes (4)
- doc/wiki/guide.md
- internal/analysis/processor/processor.go
- internal/analysis/processor/dynamic_threshold.go
- internal/conf/defaults.go
Tune default thresholds and fix bugs to reduce incorrect species identifications: - Raise dynamic threshold minimum from 0.20 to 0.40 - Raise range filter threshold from 0.01 to 0.03 - Enable false positive filter by default at level 2 (Moderate) - Slow dynamic threshold escalation to require 2/4/6 detections per level - Fix zero-threshold bug where species with threshold:0 accepted all detections - Add named constants for threshold levels and detection counts - Convert base threshold tests to table-driven subtests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeremyames-gmail
force-pushed
the
feat/false-positive-reduction
branch
from
d599d64 to
67c1fd0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/analysis/processor/base_threshold_test.go (1)
10-65: Consider parallelizing the table-driven subtests.These cases are independent, so adding
t.Parallel()at the test and subtest level should be safe and speeds up the suite.♻️ Proposed refactor
func TestGetBaseConfidenceThreshold(t *testing.T) { + t.Parallel() tests := []struct { name string globalThresh float64 speciesConfig map[string]conf.SpeciesConfig @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() p := newTestProcessor() p.Settings.BirdNET.Threshold = tt.globalThresh p.Settings.Realtime.Species = conf.SpeciesSettings{ Config: tt.speciesConfig,As per coding guidelines, "Use t.Parallel() when tests can run concurrently" and "Always call t.Parallel() within subtests if test cases are independent."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/analysis/processor/base_threshold_test.go` around lines 10 - 65, The table-driven test TestGetBaseConfidenceThreshold can be parallelized: call t.Parallel() at the top of TestGetBaseConfidenceThreshold and also inside each subtest (the anonymous func passed to t.Run) so the independent cases run concurrently; ensure the subtests still construct their own processor via newTestProcessor and set p.Settings.BirdNET.Threshold and p.Settings.Realtime.Species before calling p.getBaseConfidenceThreshold to avoid shared-state races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/analysis/processor/base_threshold_test.go`:
- Around line 10-65: The table-driven test TestGetBaseConfidenceThreshold can be
parallelized: call t.Parallel() at the top of TestGetBaseConfidenceThreshold and
also inside each subtest (the anonymous func passed to t.Run) so the independent
cases run concurrently; ensure the subtests still construct their own processor
via newTestProcessor and set p.Settings.BirdNET.Threshold and
p.Settings.Realtime.Species before calling p.getBaseConfidenceThreshold to avoid
shared-state races.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
doc/wiki/guide.mdinternal/analysis/processor/base_threshold_test.gointernal/analysis/processor/dynamic_threshold.gointernal/analysis/processor/dynamic_threshold_test.gointernal/analysis/processor/processor.gointernal/conf/config.yamlinternal/conf/defaults.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/analysis/processor/processor.go
- doc/wiki/guide.md
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
1 participant
Summary
threshold: 0(actions-only) now fall through to global threshold instead of accepting everythingAll changes are to default values only — existing user configs are unaffected. The escalation logic and zero-threshold bug fix are code changes that improve behavior for all users.
Test plan
-raceflagbase_threshold_test.gowith 3 tests covering zero-threshold bug🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Changes
Tests