-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add option to calculate segment index for an existing volume tracing #7325
Conversation
…-segment-index-button
…-segment-index-button
visible | ||
open | ||
> |
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.
There was a warning that the property visible
will be renamed to open
and the renaming would be enforced by the next major version of antd. That's why I already changed this here.
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.
Great! I left some feedback :)
@@ -1068,6 +1068,15 @@ export async function downsampleSegmentation( | |||
}, | |||
); | |||
} | |||
|
|||
export async function addSegmentIndex(annotationId: string, tracingId: string): Promise<Object> { |
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.
can you make Object
more precise?
return null; | ||
} | ||
return ( | ||
<Tooltip title="Add segment index to this layer"> |
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.
<Tooltip title="Add segment index to this layer"> | |
<Tooltip title="Add segment index to this layer. This enables certain features, such as computing segment statistics. This operation only has to be performed once and is done automatically for new annotations."> |
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.
or move it into the modal.
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.
I moved it into the modal 👍
]; | ||
const items = possibleItems.filter((el) => el); | ||
const items = possibleItems.filter((el) => el && "label" in el && el.label != null); |
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.
is this to satisfy TS? Do you know why it wasn't necessary before?
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.
Yeah, sadly this is necessary to satisfy TS. As I added a label in 624 label: this.getAddSegmentIndexButton(readableName, maybeVolumeTracing)
where getAddSegmentIndexButton
might return null, there is the option of {label: null, key: string}
which wasn't covered by the typing before :/
showAddSegmentIndexModal = (volumeTracing: VolumeTracing, layerName: string) => { | ||
this.setState({ | ||
volumeTracingToAddSegmentIndex: volumeTracing, | ||
volumeLayerNameToAddSegmentIndex: layerName, |
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.
For volume annotations, the layer name is equal to volumeTracing.tracingId
afaik. Therefore, you should only need one state variable instead of two.
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.
Afaik the tracingId is the numeric identifier (like 1-23143143-4254534-5
) but here I need the readable name given to layer. The readable name is passed to showAddSegmentIndexModal
as you can see in line 624.
To make it more explicit that the readable name is used, I renamed the variable layerName
in this method to readableName
as done in getLayerSettingsHeader
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.
This is an example tracing id I printed to the console: tracingId: "0aa32bb0-d1d6-4e34-b1f9-f88e1053c050
}} | ||
> | ||
Note that this action might take a few minutes. Afterwards, the annotation is reloaded. | ||
Also, the version history of the volume data will be reset. |
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.
this is very important and should be marked in an orange alert box or something similar.
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.
I used an Alert warning text for this 👍
for reference: https://ant.design/components/alert
open | ||
> | ||
<p> | ||
This will calculate the segment index for the volume annotation layer "{volumeLayerName}". |
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.
feel free to add a sentence like "this enables features, such as computing segment statistics."
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.
Thanks for your feedback 👣Philipp. I think I covered everything 👍
return null; | ||
} | ||
return ( | ||
<Tooltip title="Add segment index to this layer"> |
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.
I moved it into the modal 👍
]; | ||
const items = possibleItems.filter((el) => el); | ||
const items = possibleItems.filter((el) => el && "label" in el && el.label != null); |
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.
Yeah, sadly this is necessary to satisfy TS. As I added a label in 624 label: this.getAddSegmentIndexButton(readableName, maybeVolumeTracing)
where getAddSegmentIndexButton
might return null, there is the option of {label: null, key: string}
which wasn't covered by the typing before :/
showAddSegmentIndexModal = (volumeTracing: VolumeTracing, layerName: string) => { | ||
this.setState({ | ||
volumeTracingToAddSegmentIndex: volumeTracing, | ||
volumeLayerNameToAddSegmentIndex: layerName, |
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.
Afaik the tracingId is the numeric identifier (like 1-23143143-4254534-5
) but here I need the readable name given to layer. The readable name is passed to showAddSegmentIndexModal
as you can see in line 624.
To make it more explicit that the readable name is used, I renamed the variable layerName
in this method to readableName
as done in getLayerSettingsHeader
}} | ||
> | ||
Note that this action might take a few minutes. Afterwards, the annotation is reloaded. | ||
Also, the version history of the volume data will be reset. |
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.
I used an Alert warning text for this 👍
for reference: https://ant.design/components/alert
showAddSegmentIndexModal = (volumeTracing: VolumeTracing, layerName: string) => { | ||
this.setState({ | ||
volumeTracingToAddSegmentIndex: volumeTracing, | ||
volumeLayerNameToAddSegmentIndex: layerName, |
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.
This is an example tracing id I printed to the console: tracingId: "0aa32bb0-d1d6-4e34-b1f9-f88e1053c050
<Tooltip title="Add segment index to this layer"> | ||
<div onClick={() => this.showAddSegmentIndexModal(volumeTracing, readableName)}> | ||
<PlusOutlined /> | ||
Add segment index |
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.
No user will know what a "segment index" is. This is a highly technical feature name exposed to the Average Joe.
Perhaps call it "Enable Segment Statistics"?
For the record, I am not a big fan of this feature:
From my point of view, we should automatically built the index for existing volume annotations to make the segment stats shine. Whether this includes the full version history or not is up for debate. |
Ok, then the PR is put on ice for now, right? Also see discussion at https://scm.slack.com/archives/C5AKLAV0B/p1697006098538059. |
We decided to add the option for a full migration of all volume annotation layers instead of the button to add the segment index just for a single one. So I’d say this PR is supserseded by #7376 |
This PR adds a new button to the volume layer menu options that allows the user to calculate the segment index for the volume annotation.
URL of deployed dev instance (used for testing):
Steps to test:
I recommend testing locally as the backend currently sets
hasSegmentIndex
to true as default for new annotations.AnnotationService.scala
changehasSegmentIndex = Some(fallbackLayer.isEmpty)
tohasSegmentIndex = Some(false)
(in line 166)AnnotationService.scala
)Issues:
(Please delete unneeded items, merge only when none are left open)