-
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
Min-Cut on Agglomerate Graph #6361
Conversation
…:scalableminds/webknossos into agglomerate-graph-min-cut
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.
awesome! didn't test, yet, but the code itself already looks very good. still, I've left some feedback :)
testing went really well 👍 unfortunately, i could only test it on https://agglomerategraphmincut.webknossos.xyz/datasets/sample_organization/test-agglomerate-file/view and not on a big segmentation, but splitting was really fun 👍 |
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.
Nice, backend mostly LGTM, just left one comment 👍
@@ -545,7 +560,6 @@ class VolumeTracingController @Inject()( | |||
_ <- bool2Fox(request.body.length == 1) ?~> "Editable mapping update group must contain exactly one update group" | |||
updateGroup <- request.body.headOption.toFox | |||
_ <- bool2Fox(updateGroup.version == currentVersion + 1) ?~> "version mismatch" | |||
_ <- bool2Fox(updateGroup.actions.length == 1) ?~> "Editable mapping update group must contain exactly one update action" |
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 there anything else we should be aware of? E.g. are the actions applied in the order intended? Can the frontend now also send multiple actions in one group?
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’d say this should work as is. For this particular update group, the order should actually not matter, as it deletes all the edges of the cut.
Multiple actions in one group is exactly what is now possible. It still has to be one group.
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, frontend looks good 👍
Min-cut on agglomerate graph.
URL of deployed dev instance (used for testing):
Steps to test:
Backend
POST /volume/:tracingId/agglomerateGraphMinCut
expects positions of two nodes between which to calculate the min-cut.[{"segmentId1":1,"segmentId2":3,"position1":[77,77,39],"position2":[77,73,80]},{"segmentId1":1,"segmentId2":49,"position1":[77,77,39],"position2":[87,100,70]},{"segmentId1":1,"segmentId2":56,"position1":[77,77,39],"position2":[80,149,94]},{"segmentId1":1,"segmentId2":58,"position1":[77,77,39],"position2":[91,107,76]},{"segmentId1":1,"segmentId2":63,"position1":[77,77,39],"position2":[83,64,59]},{"segmentId1":1,"segmentId2":76,"position1":[77,77,39],"position2":[105,101,62]},{"segmentId1":1,"segmentId2":82,"position1":[77,77,39],"position2":[96,89,66]},{"segmentId1":1,"segmentId2":88,"position1":[77,77,39],"position2":[104,53,51]},{"segmentId1":3,"segmentId2":34,"position1":[77,73,80],"position2":[62,122,87]}]
Issues: