-
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
Iterate UI for AI job modal #7368
Conversation
conf/application.conf
Outdated
@@ -130,11 +130,11 @@ features { | |||
discussionBoard = "https://forum.image.sc/tag/webknossos" | |||
discussionBoardRequiresAdmin = false | |||
hideNavbarLogin = false | |||
isWkorgInstance = false |
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.
revert!
@@ -26,7 +26,7 @@ import { clamp, computeArrayFromBoundingBox, rgbToHex } from "libs/utils"; | |||
import { getBaseSegmentationName } from "oxalis/view/right-border-tabs/segments_tab/segments_view_helper"; | |||
import { V3 } from "libs/mjs"; | |||
import { ResolutionInfo } from "oxalis/model/helpers/resolution_info"; | |||
import { isBoundingBoxExportable } from "../action-bar/download_modal_view"; |
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.
remove class before merging
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 work and implementation of the mockup! I only left some small comments during the review. Please also see my notes from testing:
- Trying to open the modal in an annotation with only a segmentation layer, I get a
If none of the processing jobs make sense without a color layer, the "AI Analysis" button should probably be disabled and have a tooltip explaining that.
TypeError: Cannot use 'in' operator to search for 'tracingId' in undefined at getReadableNameOfVolumeLayer (starting_job_modals.tsx:90:10) at StartJobForm (starting_job_modals.tsx:531:44)
- Since the modal requires to select a bounding box and creating bounding boxes is not possible when viewing a dataset, the "AI Analysis" functionality cannot be used when viewing a dataset at the moment. Maybe there should always be an option to use the full bounding box? This way, the functionality could at least be used when viewing small datasets.
- [Minor] The images are 500x500 px² (~600 kb), but are displayed much smaller (150x150 px²) which is only ~10% (~35% on hi-res). It might make sense to provide a smaller variant. However, this might be a general issue in our code-base and it might make sense to investigate and fix this in general as part of a separate issue.
- Also, has been like this before, but when there are multiple color layers, the label for the layer selection simply reads "color". Could you please replace this with "Layer" (since the placeholder already reads "Select a color layer")
- The volume annotation materialization functionality also doesn't work for me (doesn't do anything when clicking the button). I'll have a look at what's the issue with that.
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
The required property of this rule was misleading since it is not required if a bounding box cannot be configured, but in addition seems to be ignored if a validator is set. Thus in the validator one needs to make sure to avoid letting the validation fail if a bounding box cannot be configured. Also improve validation messages for bounding box selection form item.
@dieknolle3333 I should have fixed the volume annotation materialization functionality, at least the job is triggered for me, again. Would be great if you could check as well :) The issue was a form validation regression introduced in #7357. |
@daniel-wer I tried to test the volume annotation materialization, and I just wanted to make sure that this toast is expected in my case. Thank you for looking into this so quickly! |
Yes, that's expected :) |
I downsampled the images – maybe we have to fix the sizing for more pictures later on, but I shouldn't worsen the problem 😀 |
@daniel-wer I think and hope that I addressed all of your feedback :) |
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 work, was a pleasure to look at and use. I only left two small suggestions :)
docs/automated_analysis.md
Outdated
|
||
You can launch the nuclei inferral from the `Menu` dropdown in the action bar at the top. Use the `Start AI analysis` link to start the analysis. | ||
You can launch the neuron segmentation from the action bar at the top. Use the `Start AI neuron segmentation` button in the modal to start the analysis. |
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.
You can launch the neuron segmentation from the action bar at the top. Use the `Start AI neuron segmentation` button in the modal to start the analysis. | |
You can launch the AI analysis modal using the button in the action bar at the top. Use the `Start AI neuron segmentation` button in the modal to start the analysis. |
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
thank you for the considerate review! |
Steps to test:
isWkorgInstance
andjobsEnabled
to true inapplication.conf
NB: it seemed not to be working before and still isnt, at least for me. its probably important to test this in depth during review 😅
TODOs:
Alert
componentIssues:
(Please delete unneeded items, merge only when none are left open)