Repository navigation
Fix: Species rarity lookup does not use the active geomodel taxonomy (#3973) - #3978
Conversation
- Implements GetRarityContext for atomicity under a single read lock. - Uses openfauna.CanonicalName to properly handle taxonomic aliases in species score mapping. - Removes JSON pointer assignments in API to use omitzero over omitempty.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSpecies matching, rarity computation, and taxonomy responses now use canonical scientific names, active geomodel context, and value-based JSON fields. Classifier orchestration exposes rarity context, while tests cover aliases, geomodel-only species, rarity statuses, and serialization. ChangesSpecies rarity and taxonomy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SpeciesAPI
participant Orchestrator
participant PrimaryModel
SpeciesAPI->>Orchestrator: request rarity context
Orchestrator->>PrimaryModel: read active geomodel and classifier labels
Orchestrator->>PrimaryModel: fetch probable-species scores
Orchestrator-->>SpeciesAPI: return scores and label context
SpeciesAPI->>SpeciesAPI: compute rarity score and status
Suggested labels: Suggested reviewers: 🚥 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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| Rarity SpeciesRarityInfo `json:"rarity,omitzero"` | ||
| Taxonomy ebird.TaxonomyTree `json:"taxonomy,omitzero"` |
There was a problem hiding this comment.
Bug: The omitzero struct tag is used for API response structs, but the standard encoding/json library does not support it, causing zero-value fields to be included in responses.
Severity: MEDIUM
Suggested Fix
Revert the use of omitzero for API response structs. Either continue using pointer types with the omitempty tag to omit nil values, or implement a custom JSON marshaler for the API that correctly handles the omitzero logic for value types.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: internal/api/v2/species/species.go#L108-L109
Potential issue: The pull request replaces pointer fields with `omitempty` tags with
value-type fields using `omitzero` tags in API response structs. However, the project's
API serialization relies on the standard `encoding/json` library, which does not support
the `omitzero` tag. As a result, the tag is ignored, and fields intended to be omitted
when they are zero-value will now always be included in the JSON response as empty
objects. This changes the API contract and causes a regression from the previous
behavior where `nil` pointer fields were correctly omitted from the response.
Also affects:
internal/api/v2/species/species.go:38~38
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Not applying this one. encoding/json has supported omitzero since Go 1.24, and go.mod declares go 1.26.5, so the tag is honoured rather than ignored. The repo already relies on it deliberately elsewhere, e.g. internal/notification/push_webhook.go, which documents it as Go 1.24+. No custom JSON serializer is configured, so ctx.JSON goes through the standard library.
I checked the wire format rather than assuming: I could not construct a reachable input where the emitted JSON differs from the pointer + omitempty form. SpeciesRarityInfo always has Date set on the success path so it is never the zero struct and is always emitted; on the error path the field is never assigned, stays zero, and is omitted, matching the old nil pointer exactly. Both taxonomy backends unconditionally set Kingdom: "Animalia", so no tree is ever all-zero either. There is a test pinning the omission direction now (TestSpeciesInfoJSONOmitsUnsetBlocks), which is what was actually missing.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/classifier/birdnet.go`:
- Around line 1490-1492: Update getCachedSpeciesScores and its cache lookup to
use openfauna.CanonicalName(detection.ExtractScientificName(...)) consistently
instead of strings.ToLower(...). Ensure both cache construction and retrieval
use the same canonical scientific-name key, matching the targetSci comparison in
the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e12b3858-4051-439c-acf9-e1441e51d949
📒 Files selected for processing (4)
internal/api/v2/species/species.gointernal/api/v2/species/species_test.gointernal/classifier/birdnet.gointernal/classifier/orchestrator.go
|
I think this whole PR is wrong as taxonomy lookups should be handled by OpenFauna, not by Geomodel |
tphakala
added
pkg:api
Follow-up fixes on the species rarity lookup work. Occurrence score cache: the universal geomodel labels its scores with geomodel names, so a cache keyed on the raw scientific name never matched a caller passing the classifier's legacy synonym. Every such lookup missed and fell through to a full probable-species recomputation, on the per-detection path. Cache writes, cache reads and the fallback scan now share one canonicalScoreKey helper, so both paths resolve a synonym to the same entry. GetRarityContext: it previously took two separate label snapshots and then re-resolved o.primary through Orchestrator.GetProbableSpecies, so nothing was actually consistent despite the comment claiming otherwise. It now drives every read from one primary snapshot and one settings snapshot, and takes the geomodel labels from getProbableSpecies, which already captures them under the same range-filter lock that produced the scores. Also guards the taxonomy tree dereference, corrects the speciesHasGeomodelCoverage doc comment (it reads the geomodel vocabulary now, not the primary classifier's), extracts the repeated canonical label comparison, and gofmt. Tests: cache/fallback key agreement including the legacy synonym pair, the geomodel-vocabulary precedence that is the point of the fix, the empty-geomodel fallback for the TFLite and plain ONNX backends, and GetRarityContext across universal, non-geomodel and nil-primary cases.
…nthetic scores as rarity Review of the previous commit turned up a regression it introduced, plus three gaps in the original change. Canonical-name matching merged two species pairs that BirdNET v2.4 ships separately: OpenFauna maps Dicrurus divaricatus onto Dicrurus adsimilis and Mirafra cantillans onto Mirafra javanica, so keying on the canonical name alone collapsed each pair onto one entry. The occurrence cache then reported one bird's probability for the other and disagreed with its own uncached fallback, and the species endpoint answered a request for one of them with the other's label, common name and rarity. Every match now tries the exact scientific name across the whole vocabulary before any alias, so a species that exists under its own name is never shadowed, while a legacy synonym still resolves. The occurrence cache and its fallback share one index built by buildOccurrenceIndex, so they cannot diverge again. Rarity no longer reads a synthetic score as an occurrence probability. Coverage decides first: PassUnmappedSpecies injects unmapped species at 0.0 so they survive the filter and force-includes arrive at 1.0 so they read as always active, and neither is rarity data. Previously the same species reported very_rare or unknown depending on the PassUnmappedSpecies toggle. The species endpoint now threads the matched label's scientific name to the name resolver and the taxonomy lookup instead of the raw request string. Both key on current names, so a request naming a species by a legacy synonym returned 200 with the common name unresolved and the taxonomy block silently missing. Also: drops canonicalScoreKey, which duplicated canonicalSpeciesKey in the same package, and routes the four hand-inlined copies of that expression through it; corrects the GetRarityContext doc, which claimed a reload cannot desync scores from vocabularies when only the scores and geomodel labels are lock-consistent; corrects the speciesHasGeomodelCoverage doc, which said bats are absent from the geomodel when it carries 66 bat and 42 other mammal labels. Tests: the new classifier tests published their fixture only on the instance while currentSettings resolves through the global snapshot, so they failed whenever a sibling test left settings published. They now publish globally and do not run in parallel, matching the package. Added regression coverage for both collision pairs, the label-side alias direction, synthetic scores reporting unknown, the uncached fallback, and the omitzero fields being omitted when unset. Each new test was verified to fail against the pre-fix code.
… what the fixes cover A fresh review of the previous commit found it regressed the thing it touched and that three of its comments claimed more than the code delivers. Threading the matched label's scientific name into the taxonomy and name lookups was the wrong direction. Detections are canonicalized at ingestion, so a request carries the current name while BirdNET v2.4's labels carry the legacy one, and the embedded taxonomy database is a frozen snapshot holding some species under each. Of the 236 alias pairs v2.4 ships under the legacy name, 153 are indexed under both, 56 only under the current name and 27 only under the legacy one, so preferring the label's name dropped the taxonomy block for 56 species to recover 27. Both lookups now try both names. The response also reports the requested name again rather than the matched label's, which was an unasked-for change to a public endpoint. Comment corrections, each verified against the code rather than reasoned about: - computeRarity claimed it stops both synthetic-score mechanisms being read as rarity. It stops one. resolveOverrideLabels resolves a force-include against the geomodel labels first, so a force-included species the geomodel knows is inside the coverage vocabulary and still reads as very common off its injected 1.0. Only an override outside that vocabulary reaches the unknown path. Separating a real score from an injected one needs the range filter to tag synthetic entries; vocabulary membership cannot express it. The test's second case was renamed to say what it actually exercises. - speciesHasGeomodelCoverage told callers to gate on LocationBased. That flag is false only for an unconfigured location; a range filter that failed to load leaves it true while every species reports very rare at score 0, so the guard it advertised does not exist. - The exact-name-first ordering separates an alias-merged pair only while both members are in the list. The probable-species list carries only species above today's threshold, so one member being absent is the common case, and the alias fallback then still answers for the absent one with the present one's score. Also guards the empty-key case in the occurrence index's first pass, matching the second, and makes the fallback test assert it actually reached the fallback rather than only that the result was zero.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/classifier/birdnet.go (1)
1533-1543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated clamp logic into a helper.
The cache-hit path (Lines 1511-1518) and this fallback path now duplicate the same "clamp to [0,1]" block verbatim. As per coding guidelines,
internal/**/*.gocode should "Extract common code to eliminate duplication (dupl)".♻️ Proposed extraction
+// clampOccurrence clamps an occurrence score to the valid [0.0, 1.0] range. +func clampOccurrence(v float64) float64 { + switch { + case v < 0.0: + return 0.0 + case v > 1.0: + return 1.0 + default: + return v + } +} + func (bn *BirdNET) GetSpeciesOccurrenceAtTime(species string, detectionTime time.Time) float64 { ... if err == nil && len(cachedScores) > 0 { if occurrence, found := lookupOccurrence(cachedScores, species); found { - // Clamp the score to [0.0, 1.0] range - if occurrence < 0.0 { - return 0.0 - } - if occurrence > 1.0 { - return 1.0 - } - return occurrence + return clampOccurrence(occurrence) } } ... if occurrence, found := lookupOccurrence(buildOccurrenceIndex(speciesScores), species); found { - // Clamp the score to [0.0, 1.0] range - if occurrence < 0.0 { - return 0.0 - } - if occurrence > 1.0 { - return 1.0 - } - return occurrence + return clampOccurrence(occurrence) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/classifier/birdnet.go` around lines 1533 - 1543, Extract the duplicated [0.0, 1.0] score clamping logic into a shared helper in internal/classifier/birdnet.go, then replace the inline blocks in both the cache-hit path and the fallback path around lookupOccurrence with calls to that helper. Preserve the existing behavior for values below, within, and above the range.Source: Coding guidelines
internal/api/v2/species/species_test.go (1)
718-739: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse the shared occurrence index in rarity lookup.
species.gostill re-implements exact-name-before-canonical-alias lookup withfindSpeciesScore/speciesHasGeomodelCoverage, whileclassifier/birdnet.gocentralizes the same invariant inbuildOccurrenceIndex/lookupOccurrence. Use the same index/called path fromcomputeRarityso a future change to alias collision handling can’t leave rarity behind another copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/v2/species/species_test.go` around lines 718 - 739, Update computeRarity to use the shared occurrence index and lookup path from classifier/birdnet.go, specifically buildOccurrenceIndex and lookupOccurrence, instead of the local findSpeciesScore and speciesHasGeomodelCoverage logic. Remove or bypass the duplicated rarity lookup while preserving exact-name precedence over canonical aliases and unknown status for species without geomodel coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/v2/species/species_test.go`:
- Around line 718-739: Update computeRarity to use the shared occurrence index
and lookup path from classifier/birdnet.go, specifically buildOccurrenceIndex
and lookupOccurrence, instead of the local findSpeciesScore and
speciesHasGeomodelCoverage logic. Remove or bypass the duplicated rarity lookup
while preserving exact-name precedence over canonical aliases and unknown status
for species without geomodel coverage.
In `@internal/classifier/birdnet.go`:
- Around line 1533-1543: Extract the duplicated [0.0, 1.0] score clamping logic
into a shared helper in internal/classifier/birdnet.go, then replace the inline
blocks in both the cache-hit path and the fallback path around lookupOccurrence
with calls to that helper. Preserve the existing behavior for values below,
within, and above the range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 13656314-f73e-4329-8438-bb684058eb3c
📒 Files selected for processing (6)
internal/api/v2/species/species.gointernal/api/v2/species/species_test.gointernal/classifier/birdnet.gointernal/classifier/mapped_range_filter_test.gointernal/classifier/orchestrator.gointernal/classifier/rarity_context_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/classifier/orchestrator.go
- internal/api/v2/species/species.go
The cache-hit and fallback paths of GetSpeciesOccurrenceAtTime ended up with the same clamp block verbatim once both were routed through lookupOccurrence. Behaviour is unchanged, including for NaN, which neither comparison matches and which is therefore still returned as-is.
|
@coderabbitai Thanks, both looked at. Clamp duplication (birdnet.go): valid, applied in ae5e3d5. The duplication was mine, introduced when I routed the fallback through Reusing It isn't reachable: both are unexported in More importantly the two need different answers from the same data. The invariant you're pointing at is real though, so both sites now document the same rule and cite each other, and both have regression tests using the actual colliding pairs from the vendored alias map ( |
|
Tip For best results, initiate chat on the files or code changes.
The Your reasoning for keeping 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Fixes #3973.
Species rarity was answered against the wrong vocabulary and without taxonomic alias resolution, so a species the geomodel can rank reported Unknown whenever the primary classifier could not name it, and a species named by a legacy synonym missed its score entirely.
What changed
Rarity coverage is decided against the geomodel's vocabulary, not the primary classifier's.
speciesHasGeomodelCoveragenow takes the label sets rather than the orchestrator, andOrchestrator.GetRarityContextsupplies the scores and both vocabularies from one snapshot. With the universal geomodel active this is a ~12K-species vocabulary spanning birds, bats, other mammals and insects, so a below-threshold species the classifier cannot name now correctly reads Very Rare instead of Unknown. When no universal geomodel produced the scores the geomodel vocabulary is empty and coverage falls back to the classifier's labels, which is the correct vocabulary for the TFLite meta model and the plain ONNX range filter rather than a degraded approximation.Scientific-name matching resolves taxonomic aliases, with the exact name always tried first. Every match tries the exact name across the whole vocabulary before consulting
openfauna.CanonicalName. That ordering is load-bearing: the alias map merges two pairs BirdNET v2.4 ships as separate species (Dicrurus divaricatusontoDicrurus adsimilis,Mirafra cantillansontoMirafra javanica), and canonical-first matching answered a request for one of them with the other's label, common name, rarity and occurrence probability. Separation holds while both members are in the list; when only one is, the alias fallback still answers for the absent one, which is ordinary alias behaviour rather than a guarantee.The occurrence-score cache resolves synonyms. With the universal geomodel the scores carry geomodel labels while callers pass classifier labels, so for the 236 aliased v2.4 species every lookup missed the cache, fell through to a full probable-species recomputation per detection, and then returned 0.0 anyway because the fallback compared raw names too. Cache and fallback now share one index built by
buildOccurrenceIndex, so they cannot diverge. Measured on an aliased species at 6000 probable species: 12,050 allocations and 1.42 MiB per detection before, 26 allocations and 214 KiB after.A synthetic score is no longer read as a rarity.
PassUnmappedSpeciesinjects unmapped species at 0.0 purely so they survive the filter. That was being read as Very Rare, which made the badge depend on an unrelated toggle. Coverage now decides first, so those species report Unknown. This does not cover force-includes:resolveOverrideLabelsresolves an override against the geomodel labels first, so a force-included species the geomodel knows stays inside the coverage vocabulary and still reads as Very Common off its injected 1.0. Distinguishing a real score from an injected one needs the range filter to tag synthetic entries, which is left for a follow-up.The species endpoint answers alias requests properly. The name resolver and the taxonomy backends are tried under both the requested name and the matched label's name. Neither is reliably the indexed form: requests carry the current name because detections are canonicalized on write, v2.4's labels carry the legacy one, and the embedded taxonomy snapshot holds 153 of the 236 pairs under both, 56 only under the current name and 27 only under the legacy one.
Response structs move from pointer fields with
omitemptyto value fields withomitzero. The emitted JSON is unchanged in every reachable state;omitzerohas been supported since Go 1.24 and the repo already uses it.User-visible behaviour changes
WithoutRangeDatafigure already shown in the range filter status.occurrencevalue for the 236 aliased species changes from 0.0 to its real score. It is not persisted, but it does reach the MQTT payload and theOccurrenceaction-template placeholder.Testing
Regression coverage for both collision pairs, the label-side alias direction, synthetic scores reporting Unknown, the uncached fallback, and the
omitzerofields being omitted when unset. Every new test was verified to fail against the pre-fix code and pass after.gofmtclean,golangci-lint runreports 0 issues across the project, andgo test -racepasses on the species, classifier and processor packages.Summary by CodeRabbit
Improvements
Tests