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

Add UI for segment statistics (volume and bbox) #7249

Merged
merged 61 commits into from
Sep 18, 2023

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Aug 4, 2023

Steps to test:

for a single segment

  • Go to annotation without fallback layer
  • Brush a segment
  • Right-click the segment and read its volume
  • Copy the size to clipboard

in segments tab

  • right-click on any group to open its context menu
  • select Show Segment Statistics
  • view table and download CSV

TODOs:

  • handle update of size by having context menu visibility as dependency
  • improve icon
  • implement reload button with tooltip "update this statistic", that saves the annotation
  • mock-up of size in segment tab
  • get size of all successor segments of a group recursively, not only direct children
  • add spinner while segment statistics are loading
  • make sure its the latest size, so that annotation was saved recently when the modal is opened
  • show bounding box in columns and in CSV after the route was built
  • improve naming of CSV
  • get group name
  • include bounding box info in segment context menu
  • make group actions recursive
  • check whether size in nm³ and bouding boxes are actually correct
  • maybe improve how root group is defined by not just checking for null

Issues:


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

@dieknolle3333 dieknolle3333 changed the title Segment volume route Use segment volume route in frontend Aug 4, 2023
@dieknolle3333 dieknolle3333 self-assigned this Aug 4, 2023
@fm3
Copy link
Member

fm3 commented Aug 28, 2023

@dieknolle3333 I added a draft for the route for the segment bounding box. Find it at /volume/:tracingId/segmentStatistics/boundingBox (same GET parameters as for volume).

I did not build the real implementation in the backend yet, so for now the route will always return this json:
{"topLeft":[5,5,5],"width":10,"height":20,"depth":30}

I hope to get it to return the correct numbers soon.

Note that the bounding box (just as the volume) will be in voxels in the requested mag.

@fm3
Copy link
Member

fm3 commented Aug 28, 2023

I added the backend implementation now. We may in the future decide to optimize the performance further by doing both statistics in one route or use backend-internal caching, but I didn’t do that yet.

@normanrz could you have a look at the backend changes? Or should we wait for Felix?

@fm3 fm3 requested a review from normanrz August 28, 2023 12:03
@dieknolle3333
Copy link
Contributor Author

@philippotto so it would be easily possible for me to spend another day testing and reviewing my own code. we planned that I'd finish this PR by yesterday so that I can move on to the next issue, so I fixed bigger issues that occured to me. Having this in mind I am looking forward to your feedback!

@dieknolle3333 dieknolle3333 marked this pull request as ready for review September 5, 2023 07:41
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

nice stuff :) I already left some feedback. let me know in case you have questions 🤙

@philippotto philippotto changed the title Use segment volume route in frontend Add UI for segment statistics (volume and bbox) Sep 14, 2023
@philippotto philippotto mentioned this pull request Sep 15, 2023
4 tasks
CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/context_menu.tsx Outdated Show resolved Hide resolved
const segmentStatisticsAsString = segmentInformation
.map(
(segmentInfo) =>
`${segmentInfo.segmentId},${segmentInfo.segmentName},${segmentInfo.groupId},${segmentInfo.groupName},${segmentInfo.volumeInVoxel},${segmentInfo.volumeInNm3},${segmentInfo.boundingBoxTopLeft},${segmentInfo.boundingBoxPosition}`,
Copy link
Member

Choose a reason for hiding this comment

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

I think, it's fine, but two things:

  • please add a usage for replaceAll in frontend/javascripts/libs/browser_feature_check.tsx so that we can warn users about it
  • in tsconfig you used es2022. however, you wrote above that es2021 would also suffice. I'd go with 2021 to keep things as backwards-compatible as possible for now (of course, it doesn't make a difference for the compilation output now, but other incompatibilities might sneak in later)

@philippotto
Copy link
Member

The CI currently fails. One error seems related to your code. The other might be due to the changed tsconfig. I think, it would be fine to add a @ts-ignore to that.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome :) I only changed some small details. Also, I made a performance optimization which is actually independent of your changes, but it caught my eye during testing and it was easy to realize.

Will merge as a next step 🚢

@philippotto philippotto merged commit e719bc3 into master Sep 18, 2023
2 checks passed
@philippotto philippotto deleted the segment-volume-route branch September 18, 2023 10:14
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.

Use segment statistics routes in frontend
4 participants