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

Make "Registering of All Segments for a BBox"- feature mag aware #8082

Merged
merged 12 commits into from
Sep 19, 2024

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Sep 17, 2024

URL of deployed dev instance (used for testing):

Steps to test:

  • Open an annotation, ideally with multiple segmentation layers

  • Open the bounding box tab and select "Register all segments for this bounding box"

  • The feature should work as before, registering the segments into a new segment group

  • Now use it on a very large BBox that exceeds the maximal volume in mag 1-1-1 (the limit is 512 * 512 * 512 Vx³)

  • zoom out to coarser mags and try again to register all segments, until it is possible. This proves that the volume limit is taking the current mag into account

  • Register the segments for a bounding box in multiple mags, from coarse to fine. In the corresponding segment groups, some segments from the finer mag should not be moved into the group that was created from the coarser mag. This would prove that the right mag is used to register the segments. (Context: If you register the segments for the same bbox in the same mag twice, two segment groups will be created. As segments can only be present in one group, each segment that is registered again will be moved into the latest "register all segments" segment group. So now, in our case, when a segment remains in the older group, this means that it was not registered again, and that means that the coarser mag was actually used)

  • In an annotation with multiple segmentation layers, use this feature and make sure that the currently visible segmentation layer is used. One way to check this is to create a new volume annotation layer in an annotation with another segmentation layer, brush to create one segment, and then register the segments in both segmentation layers. For the newly created segmentation layer, only the brushed segment should be registered. In the other segmentation layer, this should be different.

TODOs:

  • use current mag for volume limit
  • use segmentation in current mag to registering segments
  • use visible segmentation layer rather than first one.
    • test me!
    • fix problem that segmentation layer name cannot be found sometimes -> I dont think this had anything todo with the new issue

Issues:


(Please delete unneeded items, merge only when none are left open)

@dieknolle3333 dieknolle3333 self-assigned this Sep 17, 2024
@dieknolle3333 dieknolle3333 marked this pull request as ready for review September 18, 2024 12:54
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.

Well done, thanks a lot for tackling this! I mainly gave some feedback regarding wording, error communication in the frontend API and a mag-related edge case.

During testing I stumbled upon a use case that did not work before either I think. When invoking the registering functionality for a segmentation layer that is not editable (so has no associated volume tracing layer), the createSegmentGroup call later in the function fails:

api_latest.ts:837 Uncaught (in promise) Error: UnhandledRejection: "Error: Could not find volume tracing layer with name segmentation"
    at Fe.createSegmentGroup (api_latest.ts:837:13)
    at Fe.registerSegmentsForBoundingBox (api_latest.ts:731:21)

Feel free to tackle that in this PR or to create a followup issue.

frontend/javascripts/oxalis/api/api_latest.ts Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.ts Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.ts Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.ts Outdated Show resolved Hide resolved
@daniel-wer
Copy link
Member

Oh and thanks a lot for thinking through and documenting how to properly test this, that was very helpful 🙏

@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Sep 18, 2024

Thank you for the fast and considerate review! 🙏

Feel free to tackle that in this PR or to create a followup issue.

To be honest I stumbled across this case aswell, but I did not understand what made this layer special exactly, so thank you for pointing that out! I'll try to fix it and see how much effort it takes, maybe the lowest effort would be an appropriate error message.

@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Sep 19, 2024

I'll try to fix it and see how much effort it takes, maybe the lowest effort would be an appropriate error message.

I think the best solution is to register the segments in the root group (instead of in a newly created group) :) What do you think?

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.

Nicely done! 🎉

I think adding segments to the root group when a layer is not editable (and has no associated volume tracing), is a good solution, but could you please add a toast warning that the segment list won't be persisted across page reloads, then? You could also mention that in order to do so, the respective layer should be made editable by clicking the button right of the layer name.

Afterwards, feel free to merge 🚢

frontend/javascripts/oxalis/api/api_latest.ts Outdated Show resolved Hide resolved
@dieknolle3333 dieknolle3333 merged commit 1f9007c into master Sep 19, 2024
2 checks passed
@dieknolle3333 dieknolle3333 deleted the register-all-segments-mag-aware branch September 19, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Register all segments in this bounding box should be mag-aware
2 participants