-
Notifications
You must be signed in to change notification settings - Fork 377
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: Two steps validation for multilabel #2273
feat: Two steps validation for multilabel #2273
Conversation
); | ||
const validatedRecords = filteredRecord.map((record) => { | ||
const annotationLabels = record.currentAnnotation?.labels; |
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.
when using Optional Chaining ?
, a?b it will return undefined if b not exist in b.
So it's better to manage this case : const annotationLabels = record.currentAnnotation?.labels || null
currentAnnotation: null, | ||
annotation: { | ||
labels: annotationLabels || modelPredictionLabels || [], | ||
}, |
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.
To not put too much logic in one place:
use another variable for the condition annotationLabels || modelPredictionLabels || [],
return labels?.map((label) => ({ | ||
class: label, | ||
score: 1.0, | ||
})); |
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.
same comment as before, manage the undefined : return x?.b || null
})); | ||
// TODO: do not validate records without labels | ||
async validateLabels(labels) { | ||
let selectedAnnotation = {}; |
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 instead of let
expect(wrapper.emitted("validate")); | ||
expect(wrapper.emitted("validate")[0]).toEqual([ | ||
emittedValuesFromAnnotationButtons, | ||
]); |
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.
duplication:
Add the two expect in a function with the emmitted value as argument
super(superData); | ||
this.inputs = inputs; | ||
this.explanation = explanation; | ||
this.multi_label = multi_label; | ||
this.currentAnnotation = !currentAnnotation | ||
? this.annotation || null | ||
: currentAnnotation; |
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.
it seems that you can make this line in a shorter way:
this.currentAnnotation = !currentAnnotation || this.annotation || null
Description
Move records to PENDING status (instead of VALIDATED) when a labels change in multilabel Text Classifier
Closes #2265
See #2264
Note : this will also correct this issue #2160
Type of change
(Please delete options that are not relevant. Remember to title the PR according to the type of change)
How Has This Been Tested
(Please describe the tests that you ran to verify your changes. And ideally, reference
tests
)Checklist