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: Migrate annotations table to V2 data #1433

Merged
merged 14 commits into from
Jan 31, 2025
Merged

feat: Migrate annotations table to V2 data #1433

merged 14 commits into from
Jan 31, 2025

Conversation

bchu1
Copy link
Contributor

@bchu1 bchu1 commented Dec 24, 2024

closes #1373
closes #1020
closes #581
#1232

  • Fixes bug where wrong alignment IDs were being displayed in download modals.
  • Fixes bug where annotation name in sidebar didn't match name in table.
  • Updates most places to use Annotation_File_Shape_Type_Enum values from V2 directly.
  • We should migrate the query params for annotation shape selection to just be the annotation shape ID (instead of annotation ID plus shape type) but this PR doesn't do that.

@@ -115,16 +119,10 @@ export function DownloadOptionsContent() {
value={getTomogramName(referenceTomogram)}
/>
)}
{annotationToDownload && tomogramToDownload?.alignment && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check the tomogramToDownload?.alignment anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this code was super messed up. It was never supposed to display the alignmentId of the tomogram (also this condition never evaluated to true because it's impossible to download a single annotation and single tomogram at the same time 😭).

<AnnotationAlignmentCallout
alignmentId={tomogramToDownload.alignment.id}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see! re: tomogramToDownload.alignment.id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this condition was also just never evaluating to true (effect was that on this page you'd always see the truncated version of the callout)...

useDownloadModalContext()

const annotationFileAlignmentId =
annotationShapeToDownload?.annotationFiles.edges.find(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this something we want to deduplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should just produce on alignmentId at the end (technically all the files rn should have the same alignmentId but in a future world maybe it won't be)

Comment on lines +49 to +72
const LOADING_ANNOTATIONS: AnnotationShape[] = range(0, MAX_PER_PAGE).map(
() => ({
id: 0,
annotationFiles: {
edges: [],
},
annotation: {
annotationMethod: '',
authors: {
edges: [],
},
depositionDate: '',
id: 0,
lastModifiedDate: '',
methodLinks: {
edges: [],
},
methodType: Annotation_Method_Type_Enum.Manual,
objectId: '',
objectName: '',
releaseDate: '',
},
shapeType: Annotation_File_Shape_Type_Enum.Point,
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a different shape!

@@ -209,7 +208,7 @@ export function getDatasetsFilter({
where.runs.annotations ??= {}
where.runs.annotations.annotationShapes = {
shapeType: {
_in: objectShapeTypes as Annotation_File_Shape_Type_Enum[], // TODO(bchu): Remove typecast.
_in: objectShapeTypes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@bchu1
Copy link
Contributor Author

bchu1 commented Jan 31, 2025

@rainandbare Thanks for looking through!

@bchu1 bchu1 merged commit 1f8a76c into main Jan 31, 2025
15 checks passed
@bchu1 bchu1 deleted the bchu/annotations branch January 31, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants