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 neuron reconstruction job backend and frontend part #5922

Merged
merged 35 commits into from
Feb 10, 2022

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Dec 22, 2021

This PR adds the necessary parts to the frontend and backend to make the new nuclei reconstruction job of the worker callable.

Open TODOs:

  • The job needs a description
  • The job needs an example image

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Enable jobs via yarn enable-jobs.
  • Open a dataset in view mode.
  • In the dataset info tab open the processing jobs menu and select the nuclei reconstruction menu entry.
  • Go to the jobs list and wait for you job to finish.

Issues:

  • No issue exists in the repo for this

  • Updated (unreleased) changelog
    • I think this is not needed as this feature is still experimental, right?
  • Ready for review

@daniel-wer daniel-wer changed the title [WIP] Add nuclei reconstruction job backend and frontend part [WIP] Add neuron reconstruction job backend and frontend part Jan 3, 2022
@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I am currently working on making the bounding box of the volume that should be predicted for neurons configurable in the frontend.

The approach I went for is having another select (just like it is right now when the dataset has multiple color layers).
Instead of giving the user the possibility to enter the bounding box via numbers, which I think is quite bothersome, I went for the users bounding boxes. The UI now lets the user select one of the bounding boxes of this annotation. Do you think this is alrighty this way?

I think the advantages are that the user can / must explicitly but quite easy via the bounding box tool, select the volume for the prediction. Therefore the selected volume is first visually displayed to the user so the user can check again whether thats exactly the volume the user wants.
On the other hand, a simple number input that parses 6 numbers is better for automation.
My argument against the simple number input is that this is a UI and it should be more "visual" instead of simply entering numbers. If automation is needed, then this might be a good candidate for wk-libs?

Do you think my arguments make sense and that I chose the right approach?

Thanks for you feedback. 🐱

@philippotto
Copy link
Member

@MichaelBuessemeyer Sounds great! In the rare (?) scenario that a user happens to have 6 digits in their clipboard, they can always go to the bounding box tab to paste it there. We want to encourage the use of the bounding box tools and therefore it makes sense to reuse that. Good thinking!

@MichaelBuessemeyer
Copy link
Contributor Author

In the rare (?) scenario that a user happens to have 6 digits in their clipboard, they can always go to the bounding box tab to paste it there.

Thanks , thats a very smart solution to that problem 👍

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

@philippotto I just checked the frontend part of the neuron inferral again. It is now ready for your review. Could you please take over that review? 👀 📝

Random emoji of this post: 🐧

@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Add neuron reconstruction job backend and frontend part Add neuron reconstruction job backend and frontend part Jan 24, 2022
@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review January 24, 2022 17:45
@MichaelBuessemeyer
Copy link
Contributor Author

@fm3 could you please check the backend changes?

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend looks good 🎉

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

@philippotto I just finished applying your feedback. I also added the example screenshot for the neuron inferral. Could you please another look on this pr 👀 🏃‍♂️

Random emoji of this post: 🏎️

@@ -186,8 +187,7 @@ function StartingJobModal(props: StartingJoblModalProps) {
<br />
<div style={{ textAlign: "center" }}>
<img
//TODO: get an image
src="/assets/images/nuclei_inferral_example.jpg"
src={`/assets/images/${_.snakeCase(jobName)}_example.jpg`}
Copy link
Member

Choose a reason for hiding this comment

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

this will make it very hard to search where the file is used. I'd suggest to create a mapping object from jobName to path to make it easier to find. something like this:

const jobNameToImagePath = {
  "neuron_inferral": "neuron_inferral_example.jpg",
  ...
}

Copy link
Member

Choose a reason for hiding this comment

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

also: the screenshot quality of the new image is quite poor. maybe set the zoom value to 1 to avoid resizing issues? also, I'd hide the "crosshair" in the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I did notice that the image had such a bad resolution. It should be better blazingly sharp 🗡️ now.

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 could test it and after fixing an issue in vx, it worked very well :) I only left some cosmetic feedback for the UI. Merging this PR is blocked by the worker PR, anyway.

@fm3
Copy link
Member

fm3 commented Feb 3, 2022

Sorry I didn’t read through all of the code and comments just now, but quick question: what mechanism are you using to limit this feature to scm team members? the isSuperUser property is not currently exposed in the user json, is it?

@philippotto
Copy link
Member

Sorry I didn’t read through all of the code and comments just now, but quick question: what mechanism are you using to limit this feature to scm team members? the isSuperUser property is not currently exposed in the user json, is it?

Oh, this is a good point. It's only using activeUser.isAdmin. Could you expose the property?

@fm3
Copy link
Member

fm3 commented Feb 3, 2022

Oh, this is a good point. It's only using activeUser.isAdmin. Could you expose the property?

I added isSuperUser to the user json in 46629d6

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Could you please check the change that makes the neuron inferral only available to super users?
I think after that this pr is ready to 🚢 .

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.

LGTM :)

@MichaelBuessemeyer MichaelBuessemeyer merged commit e915b3c into master Feb 10, 2022
@MichaelBuessemeyer MichaelBuessemeyer deleted the add-nuclei-reconstruction-job branch February 10, 2022 13:28
hotzenklotz added a commit that referenced this pull request Feb 18, 2022
…ssos into docs

* 'docs' of github.com:scalableminds/webknossos:

* 'master' of github.com:scalableminds/webknossos:
  Split cells via Min Cut (#5885)
  Clean up backend util package (#6048)
  Guard against empty saves (#6052)
  Time tracking: Do not fail on empty timespans list (#6051)
  Fix clip button changing position (#6050)
  Include ParamFailure values in error chains (#6045)
  Fix non-32-aligned bucket requests (#6047)
  Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count (#5999)
  Bump cached-path-relative from 1.0.2 to 1.1.0 (#5994)
  Volume annotation download: zip with BEST_SPEED (#6036)
  Sensible scalebar values (#6034)
  Faster CircleCI builds (#6040)
  move to Google Analytics 4 (#6031)
  Fix nightly (fix tokens, upgrade puppeteer) (#6032)
  Add neuron reconstruction job backend and frontend part (#5922)
  Allow uploading multi-layer volume annotations (#6028)
hotzenklotz added a commit that referenced this pull request Feb 18, 2022
* docs:
  Split cells via Min Cut (#5885)
  Clean up backend util package (#6048)
  Guard against empty saves (#6052)
  Time tracking: Do not fail on empty timespans list (#6051)
  Fix clip button changing position (#6050)
  Include ParamFailure values in error chains (#6045)
  Fix non-32-aligned bucket requests (#6047)
  Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count (#5999)
  Bump cached-path-relative from 1.0.2 to 1.1.0 (#5994)
  Volume annotation download: zip with BEST_SPEED (#6036)
  Sensible scalebar values (#6034)
  Faster CircleCI builds (#6040)
  move to Google Analytics 4 (#6031)
  Fix nightly (fix tokens, upgrade puppeteer) (#6032)
  Add neuron reconstruction job backend and frontend part (#5922)
  Allow uploading multi-layer volume annotations (#6028)
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.

3 participants