feat: filter alerts/sequences/slack by daily fire risk score#583
feat: filter alerts/sequences/slack by daily fire risk score#583MateoLostanlen merged 36 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
+ Coverage 89.00% 89.80% +0.79%
==========================================
Files 53 55 +2
Lines 2219 2422 +203
==========================================
+ Hits 1975 2175 +200
- Misses 244 247 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fe51
left a comment
There was a problem hiding this comment.
Thanks a lot for this exploratory PR !
A preliminary review following our discussion and before we’ve looked at the test updates.
These aren’t critical comments at this stage.
Perhaps the endpoint should make it clear that the alerts retrieved are, by default, filtered based on a risk score?
| return mapping | ||
|
|
||
|
|
||
| async def _resolve_class_per_camera( |
There was a problem hiding this comment.
name shloud be more explicit, something like, fire_risk or fwi
| async def _resolve_class_per_camera( | |
| async def _resolve_FWI_class_per_camera( |
| seq_match = seq_match.where(Sequence.last_seen_at > utcnow() - timedelta(hours=24)) | ||
| seq_match = seq_match.where(Sequence.is_wildfire.is_(None)) # type: ignore[union-attr] |
There was a problem hiding this comment.
Why not ? (same approach as before)
Might even be relavant to chain with previous lines from l152
| seq_match = seq_match.where(Sequence.last_seen_at > utcnow() - timedelta(hours=24)) | |
| seq_match = seq_match.where(Sequence.is_wildfire.is_(None)) # type: ignore[union-attr] | |
| seq_match = seq_match.where(Sequence.last_seen_at > utcnow() - timedelta(hours=24)).where(Sequence.is_wildfire.is_(None)) # type: ignore[union-attr] |
There was a problem hiding this comment.
you are right better to chain , juste updated for readibily
| last_seen_at: datetime = Field(..., nullable=False) | ||
| # Highest detection confidence ever attached to this sequence. | ||
| # Monotonic: never recomputed downward when detections are deleted/reassigned. | ||
| max_conf: Union[float, None] = Field(None, nullable=True) |
There was a problem hiding this comment.
I am ok to add comment for field desc. However, would not it be the opportunity to put this comment as a postgres field desc
| max_conf: Union[float, None] = Field(None, nullable=True) | |
| max_conf: Union[float, None] = Field(None, nullable=True, description="Highest detection confidence ever attached to this sequence. Monotonic: never decremented when detections are deleted or reassigned.", | |
| ) |
fe51
left a comment
There was a problem hiding this comment.
Thanks for the updates, I might I have missed something, but, some tests might be missing
/alerts/fromdate has zero tests —> it got risk_score added same as the other endpoints, but nothing covers it. It also uses the distinct target_date code path in _resolve_class_per_camera, which is the only branch that hits risk_service.get_scores_for_date() —> that async path is not exercised by any test in the file.
Alert with mixed sequences —> no test for an alert that has one sequence above threshold and one below. The alert would appear (it matched the seq_match subquery via the passing sequence) but with only the surviving sequence in its payload. Worth having a test to pin that behavior as intentional. What do you think ?
| @pytest.mark.asyncio | ||
| async def test_unlabeled_latest_keeps_all_when_class_is_moderate( | ||
| async_client: AsyncClient, detection_session: AsyncSession, reset_risk_cache | ||
| ): | ||
| camera_id = pytest.camera_table[0]["id"] | ||
| pose_id = pytest.pose_table[0]["id"] | ||
| low_seq = await _seed_unlabeled_sequence(detection_session, camera_id, pose_id, max_conf=0.10, minutes_ago=30) | ||
|
|
||
| risk_service._scores = {camera_id: "moderate"} | ||
|
|
There was a problem hiding this comment.
I might be a bit paranoid about this filter, but I reckon it might be worth introducing tests for ‘moderate’ and above right now, so that if we update the rules for those risk levels later, they’ll already have been tested.
Below this suggestion (but not tested)
| @pytest.mark.asyncio | |
| async def test_unlabeled_latest_keeps_all_when_class_is_moderate( | |
| async_client: AsyncClient, detection_session: AsyncSession, reset_risk_cache | |
| ): | |
| camera_id = pytest.camera_table[0]["id"] | |
| pose_id = pytest.pose_table[0]["id"] | |
| low_seq = await _seed_unlabeled_sequence(detection_session, camera_id, pose_id, max_conf=0.10, minutes_ago=30) | |
| risk_service._scores = {camera_id: "moderate"} | |
| @pytest.mark.asyncio | |
| @pytest.mark.parametrize("fwi_class", ["moderate", "high", "very_high", "extreme"]) | |
| async def test_unlabeled_latest_keeps_all_when_class_is_moderate_or_above( | |
| fwi_class: str, async_client: AsyncClient, detection_session: AsyncSession, reset_risk_cache | |
| ): | |
| camera_id = pytest.camera_table[0]["id"] | |
| pose_id = pytest.pose_table[0]["id"] | |
| low_seq = await _seed_unlabeled_sequence(detection_session, camera_id, pose_id, max_conf=0.10, minutes_ago=30) | |
| risk_service._scores = {camera_id: fwi_class} | |
Why
Operators report way too many alerts on rainy / low-risk-weather days, when there is essentially no chance of an actual wildfire. The detection model still picks up smoke-like patterns (clouds, mist, exhaust) and they flood Slack and the alerts list, drowning real signal. This PR cross-references the per-camera daily Fire Weather Index from pyro-risk-api and drops low-confidence sequences when the local fire risk is
loworvery_low— the regime where false positives dominate.Summary
lowdays, drops sequences with max_conf < 0.45; onvery_low, drops below 0.6; onmoderate+(or unknown / risk-api unreachable), behaviour is unchanged./alerts/unlabeled/latest,/alerts/all/fromdate,/sequences/unlabeled/latest,/sequences/all/fromdate, and to Slack notifications at sequence creation. Alerts that lose all their sequences are dropped from the response.WHEREclause (per-cameraCASEexpression onSequence.max_conf), soLIMIT/OFFSETpagination stays exact instead of returning sparse pages./all/fromdateendpoints query the risk-api/scores/{date}?organization_id=...per request so the threshold matches the requested day; the daily cache only powers the "today" code paths.?risk_score=<fwi_class>query param that overrides the per-camera lookup and applies a single class to every sequence — useful as a kill-switch (risk_score=moderatedisables filtering) or to preview a different risk regime without touching the cache.Schema change
Adds a nullable
max_confcolumn tosequences, maintained at ingest:others_bboxesrepresent unrelated detections in the same image and would otherwise inflate the score).CASE WHEN(max_conf := GREATEST(max_conf, candidate)semantics, runs on Postgres + SQLite).Detection.bboxonly.max_confis monotonic non-decreasing — not recomputed when a detection is deleted/reassigned.Config
Adds
RISK_API_URL,RISK_API_LOGIN,RISK_API_PWD,RISK_REFRESH_HOUR_UTC(default 4) to.env.exampleanddocker-compose.yml. FWI thresholds live in code asFWI_MIN_CONF(dict inservices/risk.py) covering all 6 EFFIS classes. WhenRISK_API_URLis unset, the feature is fully disabled (fail-open).