Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sequences): create a table for easy detection aggregation #405

Merged
merged 18 commits into from
Jan 15, 2025

Conversation

frgfm
Copy link
Member

@frgfm frgfm commented Jan 9, 2025

As discussed, this PR goal is to resurrect the concept of Events from the legacy data model.
Here is how I implemented this:

Data model

I went minimal after thinking seriously about it and selected :

  • camera_id --> a stream is a set of detections from a specific camera for now
  • started_at and last_seen_at --> time range of the detections

This way, we can get the geo metadata from the camera, and the azimuth & cie from the detections. The started_at = created_at of the first detection in the sequence, and last_seen_at = created_at from the last.
I designed it this way to avoid adding event_id / sequence_id / stream_id in the detection table (which is quite "pure" for now)

The way I see this is that by fetching a stream, we can quite easily in SQL fetch all related detections. Considering time vs ID link is very useful for the next part around the creation/update logic as I though of a few options

Creation/update logic

Here is the algorithm I came up with (goal is to have limited scope but data we can trust):

  • we introduce 3 values: STREAM_RELAXATION_SECONDS (max time between two consecutive detections to be considered in the same stream), STREAM_MIN_INTERVAL_DETS (minimal number of detections within the last STREAM_MIN_INTERVAL_SECONDS to create a stream)
  • when a detection is created:
  1. We check if there is an existing stream where last_seen_at is in the last STREAM_RELAXATION_SECONDS
  2. If there is one, we add the detection to that stream by updating its "last_seen_at" to the detection timestamp
  3. If not, we check if there are at least STREAM_MIN_INTERVAL_DETS in the last STREAM_MIN_INTERVAL_SECONDS
  4. If there is, we create a stream where started_at takes the timestamp of the first fetched detection, and last_seen_at the timestamp of the current detection
  5. Otherwise we do nothing

Review suggestion

Go by commit:

  • first commit is only about the data model evolution
  • second is about the config params
  • check the changes to the detection route in the first commit
    (the others is just utils)

I started with the minimal version, and haven't implemented the tests yet. Any feedback is welcome!

@MateoLostanlen
Copy link
Member

Hello @frgfm , thanks for this PR. It's elegant, I like the idea! It meets one of my other needs too:

I think that on the platform it is necessary to display the last 10 detections to display the most recent information, but by doing that we will not have the time of the first detection, it will be the case now.

There's just one thing missing, and that's taking azimuth into account when creating your streams: a single camera returns several streams at the same time (one per viewpoint).

What's left is to imppleterment fetch_unlabeled_detections which returns:

The max 15 last unacknowledged streams received on day {from_date} and for each of these streams the last 10 detections.

@frgfm
Copy link
Member Author

frgfm commented Jan 9, 2025

There's just one thing missing, and that's taking azimuth into account when creating your streams: a single camera returns several streams at the same time (one per viewpoint).

Easy fix, no problem

What's left is to imppleterment fetch_unlabeled_detections which returns:

Sure, I wanted to cover that in another once we're all on the same page for the data model (the implementation will quite quite trivial since I've already done it twice)

@MateoLostanlen
Copy link
Member

all good then !

@RonanMorgan
Copy link
Collaborator

RonanMorgan commented Jan 9, 2025

it might be a matter of taste but I think that stream is unclear and we should call it a wildifre :)

otherwise : thanks, seems really clean

@frgfm
Copy link
Member Author

frgfm commented Jan 9, 2025

it might be a matter of taste but I think that stream is unclear and we should call it a wildfire :)

Oh yeah, I wasn't fixed on the naming of the table. I was actually thinking of sequences, which is the most accurate (remember the detections may not have been confirmed yet, and a wildfire could be spotted on multiple cameras but here it's basically a set/list/sequence of detections from the same camera)

@frgfm
Copy link
Member Author

frgfm commented Jan 9, 2025

There's just one thing missing, and that's taking azimuth into account when creating your streams: a single camera returns several streams at the same time (one per viewpoint).

@MateoLostanlen just one thing I just realized about adding azimuth to this table and the algorithm:

  • presumably, that means you can expect the same azimuth for all detections
  • but what if we have several consecutive detections from the same camera but the detections have different azimuth?

To me, this raise two questions:

  1. would be a good thing to assume that 1 sequence = 1 single azimuth? I'd say not necessarily, the wildfire can cover two azimuths 🤔
  2. now, further question about a rare case: let's say we have multiple distinct wildfires at the same time, spotted by the same camera but on two different azimuth. What do we do? Either we consider they're the same wildfire and sequence, or we split them by azimuth, OR we check whether they overlap (geometric consideration azimuth + bbox overlap in physical space)

My recommendation, considering the rarity of the last case would be to not assume we have the same azimuth in the same sequence and just not put it in the table (retrieved dynamically from the detections if needed). No split of sequence for now. What do you think?

@MateoLostanlen
Copy link
Member

@frgfm The azimuth sent is the azimuth of the center of the camera, not the one corresponding to the detection. Therefore it do not change :) The point is to identify the viewpoint here

The detection azimuth is computed by the platforme using the bbx later :)

@MateoLostanlen
Copy link
Member

If there is multiple detection on the same viewpoint, let’s have a single stream I will manage it on the platform

@frgfm
Copy link
Member Author

frgfm commented Jan 12, 2025

@frgfm The azimuth sent is the azimuth of the center of the camera, not the one corresponding to the detection.

Alright, so this means you fill the value of camera/azimuth with that in mind? (when a sequence is created, can we trust to take the value from camera.azimuth?)

I'll have everything to proceed with that response. I'll do 2 PRs: this one to correctly implement the sequence mechanism, then another to add platform-specific routes

@MateoLostanlen
Copy link
Member

Yes engine send the camera azimuth center at each detection. Therefore in case of a ptz camera we know the viewpoints that sent the detection

@frgfm
Copy link
Member Author

frgfm commented Jan 15, 2025

The detection azimuth is computed by the platforme using the bbx later :)

One last thing that got me worries (non blocking here but still): are you saying that the azimuth in the detection is not the azimuth of the camera angle when the picture was taken?
I think we should have the basic most trustable information as a priority. This would mean:

  • default azimuth of camera, even when they can rotate
  • a detection azimuth = azimuth of the camera when the picture was taken
  • the bbox-deduced azimuth has no place in those in my opinion, getting confirmation on the keypoint location on the image is important though but not the same flow, right?

@MateoLostanlen
Copy link
Member

either a static camera or a ptz camera witch patrol trough n position will send the azimuth of the center of the camera it's always the same basically

Then on platform side using the center azimuth, the camera FOV and the bbox we deduce the smoke azimuth but you don't have to worry about that on the api side

@frgfm frgfm changed the title feat(streams): create a table for easy detection aggregation feat(sequences): create a table for easy detection aggregation Jan 15, 2025
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.56%. Comparing base (ca49671) to head (7fe6607).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/app/api/api_v1/endpoints/detections.py 45.45% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #405      +/-   ##
==========================================
+ Coverage   84.85%   85.56%   +0.71%     
==========================================
  Files          35       38       +3     
  Lines         997     1053      +56     
==========================================
+ Hits          846      901      +55     
- Misses        151      152       +1     
Flag Coverage Δ
backend 85.15% <90.90%> (+0.78%) ⬆️
client 92.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frgfm frgfm marked this pull request as ready for review January 15, 2025 18:20
@frgfm frgfm merged commit 76306aa into main Jan 15, 2025
27 checks passed
@frgfm frgfm deleted the frgfm/streams branch January 15, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants