-
Notifications
You must be signed in to change notification settings - Fork 24
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 additional coordinates to segment index #7411
Conversation
@frcroth do you know what happens if I created an ND volume annotation on master, brush some, and then upgrade to this change? Will everything explode or will this be handled somehow? It might be too much to ask to add another migration for this case (there aren’t many ND annotations in production yet), but if things get into inconsistent states, maybe we should just disable the segment index features for such annotations? |
I validated that the annotations can still be viewed, but the segment index won't work anymore. Also I noticed that the front end does not provide additional coordinates for segment volume calculations leading to unhandled exceptions whenever one right clicks segments in the canvas. |
…ted segmentIndex route
I seem to be blocked, see here: https://scm.slack.com/archives/C5AKLAV0B/p1703082065289029?thread_ts=1702477845.669909&cid=C5AKLAV0B |
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.
Getting there :)
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
Outdated
Show resolved
Hide resolved
...tore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala
Outdated
Show resolved
Hide resolved
Good stuff! I added a comment about the json format. Also, do you know what happens with nd volume annotations created before this PR? Do they have a broken segment index? Or don’t they have one? Do we need a migration for them, similar to the original #7376 ? |
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.
front-end looks good to me :) testing also went well.
Well the segment index previously was invalid since all additional coordinates were mapped to the same bucket key. With this PR the previous segment index can't be used since the keys have changed. So I think if there are nd annotations with segment indices, it would be best to recalculate the segment index. Maybe we need another route for that, since addSegmentIndex doesn't do anything if the segmentIndex already exists. |
As discussed in person, we found that no productively used volume annotations on nd datasets exist yet. So let’s add an sql migration archiving (state='Finished') any (experimental) existing annotations on nd datasets. |
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.
🚀
Steps to test:
TODOs:
frontend/javascripts/oxalis/view/context_menu.tsx
, ll. 1166 and 1308usePositionsFromSegmentStats
Issues:
(Please delete unneeded items, merge only when none are left open)