-
Notifications
You must be signed in to change notification settings - Fork 0
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: find best_detections for classifications (#631) #632
Conversation
core/_cli/migrations/20240404131807-create-best-detections.ts.js
Outdated
Show resolved
Hide resolved
core/detections/best-detections.js
Outdated
@@ -22,6 +22,15 @@ const Converter = require('../../common/converter') | |||
* description: List of stream ids to limit results | |||
* in: query | |||
* type: array|string | |||
* - name: classifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* - name: classifications | |
* - name: classification_ids |
some other places, we use classifications
for classification name not ids. so better make it consistent
core/detections/best-detections.js
Outdated
@@ -83,18 +92,20 @@ router.get('/:jobId/best-detections', (req, res) => { | |||
const { jobId } = req.params | |||
const converter = new Converter(req.query, {}, true) | |||
converter.convert('streams').optional().toArray() | |||
converter.convert('classifications').optional().toArray() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converter.convert('classifications').optional().toArray() | |
converter.convert('classification_ids').optional().toArray() |
core/detections/dao/index.js
Outdated
@@ -90,7 +90,7 @@ async function defaultQueryOptions (filters = {}, options = {}) { | |||
|
|||
async function defaultBestDetectionsQueryOptions (filters, opts) { | |||
const { user, limit, offset, fields } = opts | |||
const { streams, reviewStatuses } = filters | |||
const { streams, classifications, reviewStatuses } = filters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { streams, classifications, reviewStatuses } = filters | |
const { streams, classificationIds, reviewStatuses } = filters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
core/detections/dao/index.js
Outdated
if (classifications) { | ||
byClassicication = true | ||
|
||
bestDetectionsWhere.classificationId = classifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (classifications) { | |
byClassicication = true | |
bestDetectionsWhere.classificationId = classifications | |
if (classificationIds) { | |
byClassicication = true | |
bestDetectionsWhere.classificationId = classificationIds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this migration gets run again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only somewhere where we did not have it yet (probably not)
✅ DoD
(use na when API docs (Release notes, etc) do not need to be updated)
📝 Summary
Now when "classifications" query parameter is passed to
GET /classifier-jobs/{jobId}/best-detections
or
GET /classifier-jobs/{jobId}/best-detections/summary
The returned detections will be related to specific classification_id
🛑 Problems
💡 More ideas
Write any more ideas you have