Closed frgfm closed 3 months ago
Hi @frgfm, Love this PR! It will make life easier for us and for pyronear newbies as well !
Just a few things:
To anticipate the use of camera ptz it would be preferable to have a tuple for azimuth in the camera table and to have an azimuth entry (float this time) in the detection table.
Localization is missing from the detection table too.
I'm not sure how you intend to associate a camera list with a user (sdis 7 for example). What do you have planned?
To anticipate the use of camera ptz it would be preferable to have a tuple for azimuth in the camera table and to have an azimuth entry (float this time) in the detection table.
Could you remind me of how is a camera ptz different and why the tuple?
Localization is missing from the detection table too.
It's by design: considering that at this stage, we report a detection from a camera, but we don't have more. But perhaps you mean the bounding box on the image itself? If so, I agree that we need to add it. Perhaps in the next PR to keep this one simple? Would a keypoint be enough? (I'm thinking about the usefulness, and the bounding box is linked to the type of model we use, but it's far from comprehensive, a keypoint would perhaps be more robust for the data model?)
I'm not sure how you intend to associate a camera list with a user (sdis 7 for example). What do you have planned?
cf. the region/scope followup PR I mentioned in the description. Hesitating on the name, but I think "organizations" would work:
Then the rest of the logic is easy to implement, and that doesn't complicate things too much
Basically, ptz cameras can be oriented along 3 axes. In our case, both our cameras and those of the Firefighters will patrol N predefined positions. It would therefore be interesting to have these N positions in the form of a tuple (or list of size N). Then, when the cam sends an alert, we'll add the azimuth to the detection table to find out the corresponding position. For statis cams, we'll have a list / tuple of size 1 that will always be equal to the value sent to the detection table.
Yes, I'm talking about the bounding box, which is called localization in the current table, but I agree that the name can be changed. I suggest you keep what we have at the moment, a string that allows you to send any form of points (bbox, keypoints etc ...)
Ok understood for the organization
Yes, don't worry about adding the bbox in a next pr as long as it's present when you deploy it. It's essential information for firefighters to speed up treatment time.
Attention: Patch coverage is 88.74296%
with 60 lines
in your changes missing coverage. Please review.
Project coverage is 88.00%. Comparing base (
767be30
) to head (44506bf
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Here are the last edits:
The test suite is considerably faster than before :sweat_smile:
Suggestions for review considering the size of the PR (most of the diff is from the poetry.lock though):
Things to add to prevent feature reduction compared to the previous version:
cf #304
Following up on #304, this PR introduces the following modifications:
data model
update all routes
update dependencies, the end to end test, the CI
simplify docker orchestration by using localstack by default
noticeable changes: the camera aren't "users" anymore, but an admin can create an unexpiring token for cameras (that means they can use a created token, but don't have credentials to log in). This reduces API calls & problems with creds
The only things missing are:
In the following PRs, this is what will be added:
Any feedback is welcome!