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.

Sign up for GitHub

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

Fix bug where annotations could be deleted without warning #149

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Fix bug where annotations could be deleted without warning #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/Jobs/ApplyLargoSession.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ protected function handleImageAnnotations()
{
[$dismissed, $changed] = $this->ignoreDeletedLabels($this->dismissedImageAnnotations, $this->changedImageAnnotations);

// This is essential, otherwise annotations could be deleted without warning
// below (because changes are applied first, then labels are dismissed, then
// dangling annotations are deleted)!
[$dismissed, $changed] = $this->ignoreNullChanges($dismissed, $changed);

// Change labels first, then dismiss to keep the opportunity to copy feature
// vectors. If labels are deleted first, the feature vectors will be immediately
// deleted, too, and nothing can be copied any more.
Expand All @@ -161,6 +166,11 @@ protected function handleVideoAnnotations()
{
[$dismissed, $changed] = $this->ignoreDeletedLabels($this->dismissedVideoAnnotations, $this->changedVideoAnnotations);

// This is essential, otherwise annotations could be deleted without warning
// below (because changes are applied first, then labels are dismissed, then
// dangling annotations are deleted)!
[$dismissed, $changed] = $this->ignoreNullChanges($dismissed, $changed);

// Change labels first, then dismiss to keep the opportunity to copy feature
// vectors. If labels are deleted first, the feature vectors will be immediately
// deleted, too, and nothing can be copied any more.
Expand Down Expand Up @@ -207,6 +217,35 @@ protected function ignoreDeletedLabels($dismissed, $changed)
return [$dismissed, $changed];
}

/**
* Removes changes to annotations where the same label was dismissed than should be
* attached again later.
*/
protected function ignoreNullChanges(array $dismissed, array $changed): array
{
foreach ($dismissed as $labelId => $annotationIds) {
if (array_key_exists($labelId, $changed)) {
$toIgnore = [];
foreach ($annotationIds as $id) {
if (array_search($id, $changed[$labelId]) !== false) {
$toIgnore[] = $id;
}
}

$dismissed[$labelId] = array_filter($annotationIds,
fn ($x) => !in_array($x, $toIgnore));
$changed[$labelId] = array_filter($changed[$labelId],
fn ($x) => !in_array($x, $toIgnore));
}
}

// Remove elements that may now be emepty.
$dismissed = array_filter($dismissed);
$changed = array_filter($changed);

return [$dismissed, $changed];
}

/**
* Detach annotation labels that were dismissed in a Largo session.
*
Expand Down
52 changes: 52 additions & 0 deletions tests/Jobs/ApplyLargoSessionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,58 @@ public function testChangeVideoAnnotationCopyFeatureVector()
$this->assertEquals($l1->id, $vectors[0]->label_id);
$this->assertEquals($vector1->vector, $vectors[0]->vector);
}

public function testDismissButChangeBackToSameLabelImage()
{
$user = UserTest::create();
$image = ImageTest::create();
$a1 = ImageAnnotationTest::create(['image_id' => $image->id]);
$al1 = ImageAnnotationLabelTest::create([
'annotation_id' => $a1->id,
'user_id' => $user->id,
]);

$a2 = ImageAnnotationTest::create(['image_id' => $image->id]);
$al2 = ImageAnnotationLabelTest::create([
'annotation_id' => $a2->id,
'user_id' => $user->id,
'label_id' => $al1->label_id,
]);

$dismissed = [$al1->label_id => [$a1->id, $a2->id]];
$changed = [$al1->label_id => [$a1->id, $a2->id]];
$job = new ApplyLargoSession('job_id', $user, $dismissed, $changed, [], [], false);
$job->handle();

$this->assertNotNull($a1->fresh());
$this->assertNotNull($a2->fresh());
}

public function testDismissButChangeBackToSameLabelVideo()
{
$user = UserTest::create();
$video = VideoTest::create();
$a1 = VideoAnnotationTest::create(['video_id' => $video->id]);
$al1 = VideoAnnotationLabelTest::create([
'annotation_id' => $a1->id,
'user_id' => $user->id,
]);

$a2 = VideoAnnotationTest::create(['video_id' => $video->id]);
$al2 = VideoAnnotationLabelTest::create([
'annotation_id' => $a2->id,
'user_id' => $user->id,
'label_id' => $al1->label_id,
]);

$dismissed = [$al1->label_id => [$a1->id, $a2->id]];
$changed = [$al1->label_id => [$a1->id, $a2->id]];
$job = new ApplyLargoSession('job_id', $user, [], [], $dismissed, $changed, false);
$job->handle();

$this->assertNotNull($a1->fresh());
$this->assertNotNull($a2->fresh());
}
}

class ApplyLargoSessionStub extends ApplyLargoSession
Expand Down