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

In proofreading, split a segment from all its neighbors #7611

Merged
merged 31 commits into from
Mar 5, 2024

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Feb 7, 2024

This PR implements a new "Split from all neighboring segments" feature for the proofreading mode. The front-end code regarding the proofreading was also refactored quite a bit to generalize a bit better (the new feature only requires one agglomerate ID as input instead of two and in contrast to the other split actions, the result can consist of more than 2 new agglomerates).

URL of deployed dev instance (used for testing):

Steps to test:

  • create a new annotation for a DS with an agglomerate file
  • enable a hdf5 mapping
  • use proofreading tool
    • without skeletons:
      • rightclick a segment and select "Split from all neighboring segments" --> segment should be split as expected
    • with skeletons
      • load an agglomerate tree
      • rightclick a node and select "Split from all neighboring segments" --> segment should be split as expected
  • also test the other proofreading tools (merge, cut, min-cut)

TODOs:

  • Backend
    • Add neighbors route POST /tracings/volume/<id>/agglomerateGraphNeighbors
      • Example body {"segmentPosition":[100,119,95],"mag":[1,1,1],"agglomerateId":1,"editableMappingId":"15d8d14c-1145-43f8-80a8-9a1a065facc7"}
      • Returns {"segmentId":3,"neighbors":[{"segmentId":1,"position":[77,77,39]},{"segmentId":49,"position":[87,100,70]},{"segmentId":56,"position":[80,149,94]},{"segmentId":58,"position":[91,107,76]},{"segmentId":63,"position":[83,64,59]},{"segmentId":76,"position":[105,101,62]},{"segmentId":82,"position":[96,89,66]},{"segmentId":87,"position":[100,50,62]},{"segmentId":88,"position":[104,53,51]},{"segmentId":101,"position":[110,50,59]},{"segmentId":173,"position":[146,148,82]}]}
  • Frontend
    • User interaction (without skeletons)
      • Call the new api route, construct relevant (directed) edges from result for split actions, push those
    • User interaction from skeleton

Issues:


@fm3 fm3 self-assigned this Feb 7, 2024
@philippotto philippotto changed the title WIP: In proofreading, split a segment from all its neighbors In proofreading, split a segment from all its neighbors Feb 19, 2024
@philippotto philippotto marked this pull request as ready for review February 19, 2024 10:42
@philippotto philippotto mentioned this pull request Feb 19, 2024
11 tasks
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Thank you for the miscellaneous clean ups in the proofreading saga you did while you were at it 🥇 I wasn't able to properly test the new functionality, because I might have misunderstood how it's supposed to work or because there was a regression and it no longer works as intended.

Notes from testing:

  • If no editable mapping exists yet, I get this saga-crashing error during the first proofreading action regardless if I use split-from-all or min-cut (maybe the mapping creation is not properly awaited?)
    image
  • If I rightclick on a segment and use the new "Split from all ..." functionality in the context menu, the corresponding oversegmentation-segment is not split from all of its "neighbors". If I execute it repeatedly more and more other segments are split off (looks like about one segment per click). Am I doing it wrong or is there an issue? (Same when I do it using the agglomerate tree)
  • When using the new functionality on a 1-segment/node agglomerate, the error toast message contains a null and could be improved to state that the segment is already split ("Unable to find all nodes for positions 66,133,47null in explorative_2024-02-26_Sample_User_003.")

@fm3
Copy link
Member Author

fm3 commented Feb 29, 2024

I did some testing and I can reproduce all of the things Daniel mentioned. On first sight, it looks like the new backend route works as intended, but not all splits are sent as update actions by the frontend. Please let me know if you need anything else from my side :)

@fm3 fm3 requested a review from frcroth March 4, 2024 09:49
Copy link
Member

@frcroth frcroth left a comment

Choose a reason for hiding this comment

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

Backend LGTM

@philippotto
Copy link
Member

Thanks for the thorough testing, @daniel-wer! @fm3 and I fixed a bug each.

When using the new functionality on a 1-segment/node agglomerate, the error toast message contains a null and could be improved to state that the segment is already split ("Unable to find all nodes for positions 66,133,47null in explorative_2024-02-26_Sample_User_003.")

I cannot reproduce this. Maybe this was related to the other bugs? I don't know. Could you give it another try and tell me how to reproduce this in case you run into it again?

@fm3
Copy link
Member Author

fm3 commented Mar 4, 2024

I cannot reproduce this. Maybe this was related to the other bugs? I don't know. Could you give it another try and tell me how to reproduce this in case you run into it again?

Cannot reproduce it on the current code anymore either. It’s just a no-op now, which is correct. However, maybe an info toast that nothing happened because this segment has no neighbors would be helpful so the user doesn’t wait for something to happen.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Works very well! LGTM 👍

I cannot reproduce this. Maybe this was related to the other bugs? I don't know. Could you give it another try and tell me how to reproduce this in case you run into it again?

I also cannot reproduce it any longer and second @fm3's suggestion to show a toast in that case :)

@philippotto philippotto enabled auto-merge (squash) March 5, 2024 15:47
@philippotto philippotto merged commit fcf1655 into master Mar 5, 2024
2 checks passed
@philippotto philippotto deleted the split-from-all branch March 5, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split supervoxel from all other segments
4 participants