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

Evaluate segmentation in infer neurons task #8221

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
[Commits](https://github.com/scalableminds/webknossos/compare/24.11.1...HEAD)

### Added
- It is now possible to start a split-merger evaluation when starting a neuron inference. [#8221](https://github.com/scalableminds/webknossos/pull/8221)
- When exploring remote URIs pasted from Neuroglancer, the format prefixes like `precomputed://` are now ignored, so users don’t have to remove them. [#8195](https://github.com/scalableminds/webknossos/pull/8195)

### Changed
Expand Down
14 changes: 13 additions & 1 deletion app/controllers/JobController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,13 @@ class JobController @Inject()(
datasetName: String,
layerName: String,
bbox: String,
newDatasetName: String): Action[AnyContent] =
newDatasetName: String,
doEvaluation: Boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't know what kind of evaluation would be performed in case doEvaluation is set to true. (As I do not have work with voxelytics regularly) Therefore, I would prefer to make the name more explicit

Suggested change
doEvaluation: Boolean,
doSplitMergerEvaluation: Boolean,

annotationId: Option[String],
evalUseSparseTracing: Option[Boolean],
evalMaxEdgeLength: Option[String],
evalSparseTubeThresholdNm: Option[String],
evalMinMergerPathLengthNm: Option[String]): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
log(Some(slackNotificationService.noticeFailedJobRequest)) {
for {
Expand All @@ -248,6 +254,12 @@ class JobController @Inject()(
"new_dataset_name" -> newDatasetName,
"layer_name" -> layerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a check that all optional params are provided in case doSplitMergerEvaluation is true:

I think this can be done within the for with something like this.

annotationIdParsed <- Fox.runIf(doSplitMergerEvaluation)(annotationId.toFox)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still missing 🙈

"bbox" -> bbox,
"do_evaluation" -> doEvaluation,
Copy link
Contributor

Choose a reason for hiding this comment

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

same renaming here

Suggested change
"do_evaluation" -> doEvaluation,
"do_split_merger_evaluation" -> doSplitMergerEvaluation,

"annotation_id" -> annotationId,
"eval_use_sparse_tracing" -> evalUseSparseTracing,
"eval_max_edge_length" -> evalMaxEdgeLength,
"eval_sparse_tube_threshold_nm" -> evalSparseTubeThresholdNm,
"eval_min_merger_path_length_nm" -> evalMinMergerPathLengthNm,
)
job <- jobService.submitJob(command, commandArgs, request.identity, dataset._dataStore) ?~> "job.couldNotRunNeuronInferral"
js <- jobService.publicWrites(job)
Expand Down
3 changes: 3 additions & 0 deletions app/models/job/Job.scala
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ case class Job(
def datasetName: Option[String] = argAsStringOpt("dataset_name")

private def argAsStringOpt(key: String) = (commandArgs \ key).toOption.flatMap(_.asOpt[String])
private def argAsBooleanOpt(key: String) = (commandArgs \ key).toOption.flatMap(_.asOpt[Boolean])
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks unformatted: To format the backend code you can use yarn format-backend and to lint the backend code you can use yarn lint-backend


def resultLink(organizationId: String): Option[String] =
if (effectiveState != JobState.SUCCESS) None
Expand All @@ -63,6 +64,8 @@ case class Job(
}
case JobCommand.export_tiff | JobCommand.render_animation =>
Some(s"/api/jobs/${this._id}/export")
case JobCommand.infer_neurons if this.argAsBooleanOpt("do_evaluation").getOrElse(false) =>
returnValue.map { resultAnnotationLink => resultAnnotationLink}
case JobCommand.infer_nuclei | JobCommand.infer_neurons | JobCommand.materialize_volume_annotation |
JobCommand.infer_with_model | JobCommand.infer_mitochondria | JobCommand.align_sections =>
returnValue.map { resultDatasetName =>
Expand Down
2 changes: 1 addition & 1 deletion conf/webknossos.latest.routes
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ POST /jobs/run/computeMeshFile/:organizationId/:datasetName
POST /jobs/run/computeSegmentIndexFile/:organizationId/:datasetName controllers.JobController.runComputeSegmentIndexFileJob(organizationId: String, datasetName: String, layerName: String)
POST /jobs/run/exportTiff/:organizationId/:datasetName controllers.JobController.runExportTiffJob(organizationId: String, datasetName: String, bbox: String, additionalCoordinates: Option[String], layerName: Option[String], mag: Option[String], annotationLayerName: Option[String], annotationId: Option[String], asOmeTiff: Boolean)
POST /jobs/run/inferNuclei/:organizationId/:datasetName controllers.JobController.runInferNucleiJob(organizationId: String, datasetName: String, layerName: String, newDatasetName: String)
POST /jobs/run/inferNeurons/:organizationId/:datasetName controllers.JobController.runInferNeuronsJob(organizationId: String, datasetName: String, layerName: String, bbox: String, newDatasetName: String)
POST /jobs/run/inferNeurons/:organizationId/:datasetName controllers.JobController.runInferNeuronsJob(organizationId: String, datasetName: String, layerName: String, bbox: String, newDatasetName: String, doEvaluation: Boolean, annotationId: Option[String],evalUseSparseTracing: Option[Boolean],evalMaxEdgeLength: Option[String],evalSparseTubeThresholdNm: Option[String],evalMinMergerPathLengthNm: Option[String])
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable renaming from above and adding spaces between param definition

Suggested change
POST /jobs/run/inferNeurons/:organizationId/:datasetName controllers.JobController.runInferNeuronsJob(organizationId: String, datasetName: String, layerName: String, bbox: String, newDatasetName: String, doEvaluation: Boolean, annotationId: Option[String],evalUseSparseTracing: Option[Boolean],evalMaxEdgeLength: Option[String],evalSparseTubeThresholdNm: Option[String],evalMinMergerPathLengthNm: Option[String])
POST /jobs/run/inferNeurons/:organizationId/:datasetName controllers.JobController.runInferNeuronsJob(organizationId: String, datasetName: String, layerName: String, bbox: String, newDatasetName: String, doSplitMergerEvaluation: Boolean, annotationId: Option[String], evalUseSparseTracing: Option[Boolean], evalMaxEdgeLength: Option[String], evalSparseTubeThresholdNm: Option[String], evalMinMergerPathLengthNm: Option[String])

POST /jobs/run/inferMitochondria/:organizationId/:datasetName controllers.JobController.runInferMitochondriaJob(organizationId: String, datasetName: String, layerName: String, bbox: String, newDatasetName: String)
POST /jobs/run/alignSections/:organizationId/:datasetName controllers.JobController.runAlignSectionsJob(organizationId: String, datasetName: String, layerName: String, newDatasetName: String, annotationId: Option[String])
POST /jobs/run/materializeVolumeAnnotation/:organizationId/:datasetName controllers.JobController.runMaterializeVolumeAnnotationJob(organizationId: String, datasetName: String, fallbackLayerName: String, annotationId: String, annotationType: String, newDatasetName: String, outputSegmentationLayerName: String, mergeSegments: Boolean, volumeLayerName: Option[String])
Expand Down
14 changes: 14 additions & 0 deletions frontend/javascripts/admin/api/jobs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,12 +193,26 @@ export function startNeuronInferralJob(
layerName: string,
bbox: Vector6,
newDatasetName: string,
doEvaluation: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
doEvaluation: boolean,
doSplitMergerEvaluation: boolean,

annotationId?: string,
useSparseTracing?: boolean,
evalMaxEdgeLength?: string,
evalSparseTubeThresholdNm?: string,
evalMinMergerPathLengthNm?: string,
cdfhalle marked this conversation as resolved.
Show resolved Hide resolved
): Promise<APIJob> {
const urlParams = new URLSearchParams({
layerName,
bbox: bbox.join(","),
newDatasetName,
doEvaluation: doEvaluation.toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
doEvaluation: doEvaluation.toString(),
doSplitMergerEvaluation: doSplitMergerEvaluation.toString(),

And so on 🙈

});
if (doEvaluation) {
urlParams.append("annotationId", `${annotationId}`);
urlParams.append("evalUseSparseTracing", `${useSparseTracing}`);
urlParams.append("evalMaxEdgeLength", `${evalMaxEdgeLength}`);
urlParams.append("evalSparseTubeThresholdNm", `${evalSparseTubeThresholdNm}`);
urlParams.append("evalMinMergerPathLengthNm", `${evalMinMergerPathLengthNm}`);
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add parameter validation for evaluation settings.

The code should validate optional parameters before appending them to prevent potential undefined values from being sent to the API.

   if (doEvaluation) {
+    if (!annotationId) {
+      throw new Error("annotationId is required when doEvaluation is true");
+    }
     urlParams.append("annotationId", `${annotationId}`);
-    urlParams.append("evalUseSparseTracing", `${useSparseTracing}`);
-    urlParams.append("evalMaxEdgeLength", `${evalMaxEdgeLength}`);
-    urlParams.append("evalSparseTubeThresholdNm", `${evalSparseTubeThresholdNm}`);
-    urlParams.append("evalMinMergerPathLengthNm", `${evalMinMergerPathLengthNm}`);
+    if (useSparseTracing != null) {
+      urlParams.append("evalUseSparseTracing", `${useSparseTracing}`);
+    }
+    if (evalMaxEdgeLength != null) {
+      urlParams.append("evalMaxEdgeLength", `${evalMaxEdgeLength}`);
+    }
+    if (evalSparseTubeThresholdNm != null) {
+      urlParams.append("evalSparseTubeThresholdNm", `${evalSparseTubeThresholdNm}`);
+    }
+    if (evalMinMergerPathLengthNm != null) {
+      urlParams.append("evalMinMergerPathLengthNm", `${evalMinMergerPathLengthNm}`);
+    }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
doEvaluation: doEvaluation.toString(),
});
if (doEvaluation) {
urlParams.append("annotationId", `${annotationId}`);
urlParams.append("evalUseSparseTracing", `${useSparseTracing}`);
urlParams.append("evalMaxEdgeLength", `${evalMaxEdgeLength}`);
urlParams.append("evalSparseTubeThresholdNm", `${evalSparseTubeThresholdNm}`);
urlParams.append("evalMinMergerPathLengthNm", `${evalMinMergerPathLengthNm}`);
}
doEvaluation: doEvaluation.toString(),
});
if (doEvaluation) {
if (!annotationId) {
throw new Error("annotationId is required when doEvaluation is true");
}
urlParams.append("annotationId", `${annotationId}`);
if (useSparseTracing != null) {
urlParams.append("evalUseSparseTracing", `${useSparseTracing}`);
}
if (evalMaxEdgeLength != null) {
urlParams.append("evalMaxEdgeLength", `${evalMaxEdgeLength}`);
}
if (evalSparseTubeThresholdNm != null) {
urlParams.append("evalSparseTubeThresholdNm", `${evalSparseTubeThresholdNm}`);
}
if (evalMinMergerPathLengthNm != null) {
urlParams.append("evalMinMergerPathLengthNm", `${evalMinMergerPathLengthNm}`);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its ok to do this suggestion, but this isn't a must imo.

The backend should at last realize some params are missing (see my comment above)

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelBuessemeyer, understood. If the backend handles missing parameters, additional validation on the frontend is indeed optional.


✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: frontend/javascripts/admin/api/jobs.ts:207-215
Timestamp: 2024-11-25T14:52:20.691Z
Learning: In the webknossos codebase, it's acceptable to rely on the backend to handle missing parameters, so additional parameter validation in the frontend is optional.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

return Request.receiveJSON(
`/api/jobs/run/inferNeurons/${organizationId}/${datasetName}?${urlParams.toString()}`,
{
Expand Down
108 changes: 105 additions & 3 deletions frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import {
Switch,
type FormInstance,
Checkbox,
Collapse,
Col,
InputNumber,
} from "antd";
import {
startNucleiInferralJob,
Expand Down Expand Up @@ -98,6 +101,7 @@ type StartJobFormProps = Props & {
jobApiCall: (arg0: JobApiCallArgsType, form: FormInstance<any>) => Promise<void | APIJob>;
jobName: keyof typeof jobNameToImagePath;
description: React.ReactNode;
jobSpecificInputFields?: React.ReactNode | undefined;
isBoundingBoxConfigurable?: boolean;
chooseSegmentationLayer?: boolean;
suggestedDatasetSuffix: string;
Expand Down Expand Up @@ -523,11 +527,84 @@ function ShouldUseTreesFormItem() {
);
}

cdfhalle marked this conversation as resolved.
Show resolved Hide resolved
function CollapsibleEvaluationSettings({
cdfhalle marked this conversation as resolved.
Show resolved Hide resolved
isActive = false,
setActive,
}: { isActive: boolean; setActive: (active: boolean) => void }) {
return (
<Collapse
style={{ marginBottom: 8 }}
onChange={() => setActive(!isActive)}
expandIcon={() => <Checkbox checked={isActive} />}
items={[
{
key: "evaluation",
label: "Evaluation Settings",
children: (
<Row>
<Col style={{ width: "100%" }}>
<Form.Item
Copy link
Contributor

Choose a reason for hiding this comment

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

This from item with the check box looks a little quirky in vertical mode. Could you please make it horizontal?

<Form.Item
  ...
  layout="horizontal"
  />

Copy link
Author

@cdfhalle cdfhalle Nov 26, 2024

Choose a reason for hiding this comment

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

It seems like the layout parameter only exists for Form but not Form.Item. Do you have any Ideas on how to work around that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I am sorry, I mislooked regarding since what version this property is available. I think I read 4.18 but it is 5.18. I updated antd a little to support this prop, please pull again and run yarn install. This should update the version of antd so that the form items support the layout prop.
Maybe your IDE needs a restart after the update because the TS background process did not register the upgrade and still thinks the code is wrong. At least that's what happened in my case.

Copy link
Contributor

Choose a reason for hiding this comment

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

And you can find the docs about the layout prop here: https://ant.design/components/form#formitem. In the version column it clearly says 5.18 (not 4.18)

label="Use sparse ground truth tracing"
name={["evaluationSettings", "useSparseTracing"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also rename the name path everywhere from "evaluationSettings" to "splitMergerEvaluationSettings"

Copy link
Contributor

Choose a reason for hiding this comment

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

But as this is within the CollapsibleSplitMergerEvaluationSettings component, this might already be guessable / rather strait forward. So please only adapt if you want to :)

valuePropName="checked"
initialValue={false}
tooltip="The evaluation mode can either be `dense`
in case all processes in the volume are annotated in the ground-truth.
If not, use the `sparse` mode."
Comment on lines +559 to +561
Copy link
Contributor

Choose a reason for hiding this comment

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

These tooltip text are out of my range of expertise. => I cannot give you feedback on these

Copy link
Author

Choose a reason for hiding this comment

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

They are directly taken from the voxelytics task that does the evaluation, so they should be somewhat right.

>
<Checkbox style={{ width: "100%" }} />
</Form.Item>
<Form.Item
label="Max edge length in nm"
name={["evaluationSettings", "maxEdgeLength"]}
tooltip="Ground truth tracings can be densified so that
nodes are at most max_edge_length nm apart.
However, this can also introduce wrong nodes in curved processes."
>
<InputNumber style={{ width: "100%" }} placeholder="None" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you use a InputNumber which leads to maxEdgeLength being of type number :) -> Thus the suggestion to improve the typing to avoid potential errors in the future

</Form.Item>
<Form.Item
label="Sparse tube threshold in nm"
name={["evaluationSettings", "sparseTubeThresholdInNm"]}
tooltip="Tube threshold for sparse evaluation,
determining if a process is too far from the ground-truth."
>
<InputNumber style={{ width: "100%" }} placeholder="1000" />
</Form.Item>
<Form.Item
label="Sparse minimum merger path length in nm"
name={["evaluationSettings", "minimumMergerPathLengthInNm"]}
tooltip="Minimum ground truth path length of a merger component
to be counted as a relevant merger (for sparse evaluation).
Note, the path length to neighboring nodes of a component is included for this comparison. This optimistic path length
estimation makes sure no relevant mergers are ignored."
>
<InputNumber style={{ width: "100%" }} placeholder="800" />
</Form.Item>
<Form.Item name="useAnnotation" initialValue={true} />
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Empty Form.Item without a child component

The Form.Item at line 584 does not contain any input or form control as its child. Form.Item is intended to wrap form controls like inputs, checkboxes, or selectors. An empty Form.Item may lead to unexpected behavior and should be avoided. Please ensure that you include the appropriate form control within this Form.Item.

Apply this diff to address the issue:

-                <Form.Item name="useAnnotation" initialValue={true} />
+                {/* Remove this empty Form.Item or add a child component if needed */}

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jup, the AI is correct here. But it seems you simply want the form data to always have useAnnotation=true in case an evaluation is done, correct? In that case, I'd suggest to make the form item hidden:

Suggested change
<Form.Item name="useAnnotation" initialValue={true} />
<Form.Item name="useAnnotation" initialValue={true} hidden />

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelBuessemeyer, thank you for the clarification. Yes, making the Form.Item hidden with initialValue={true} is a good solution to ensure that useAnnotation is always set to true when evaluation is done, without rendering an empty form item. I've noted this for future reviews.


✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:584-584
Timestamp: 2024-11-25T14:52:23.511Z
Learning: When evaluation is done, ensure that `useAnnotation` is always set to `true` in the form data by using a hidden `Form.Item` with `initialValue={true}`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

</Col>
</Row>
),
},
]}
activeKey={isActive ? "evaluation" : []}
/>
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Potential issue with Collapse component's onChange handler

The onChange handler for the Collapse component receives the active key(s) as arguments. Currently, the code toggles isActive using !isActive without considering these arguments, which might lead to synchronization issues between the Collapse state and the isActive state. It is recommended to use the provided key parameter to update the isActive state accurately.

Apply this diff to improve the implementation:

        <Collapse
          style={{ marginBottom: 8 }}
-         onChange={() => setActive(!isActive)}
+         onChange={(key) => setActive(key.includes("evaluation"))}
          expandIcon={() => <Checkbox checked={isActive} />}
          items={[
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Collapse
style={{ marginBottom: 8 }}
onChange={() => setActive(!isActive)}
expandIcon={() => <Checkbox checked={isActive} />}
items={[
{
key: "evaluation",
label: "Evaluation Settings",
children: (
<Row>
<Col style={{ width: "100%" }}>
<Form.Item
label="Use sparse ground truth tracing"
name={["evaluationSettings", "useSparseTracing"]}
valuePropName="checked"
initialValue={false}
tooltip="The evaluation mode can either be `dense`
in case all processes in the volume are annotated in the ground-truth.
If not, use the `sparse` mode."
>
<Checkbox style={{ width: "100%" }} />
</Form.Item>
<Form.Item
label="Max edge length in nm"
name={["evaluationSettings", "maxEdgeLength"]}
tooltip="Ground truth tracings can be densified so that
nodes are at most max_edge_length nm apart.
However, this can also introduce wrong nodes in curved processes."
>
<InputNumber style={{ width: "100%" }} placeholder="None" />
</Form.Item>
<Form.Item
label="Sparse tube threshold in nm"
name={["evaluationSettings", "sparseTubeThresholdInNm"]}
tooltip="Tube threshold for sparse evaluation,
determining if a process is too far from the ground-truth."
>
<InputNumber style={{ width: "100%" }} placeholder="1000" />
</Form.Item>
<Form.Item
label="Sparse minimum merger path length in nm"
name={["evaluationSettings", "minimumMergerPathLengthInNm"]}
tooltip="Minimum ground truth path length of a merger component
to be counted as a relevant merger (for sparse evaluation).
Note, the path length to neighboring nodes of a component is included for this comparison. This optimistic path length
estimation makes sure no relevant mergers are ignored."
>
<InputNumber style={{ width: "100%" }} placeholder="800" />
</Form.Item>
<Form.Item name="useAnnotation" initialValue={true} />
</Col>
</Row>
),
},
]}
activeKey={isActive ? "evaluation" : []}
/>
<Collapse
style={{ marginBottom: 8 }}
onChange={(key) => setActive(key.includes("evaluation"))}
expandIcon={() => <Checkbox checked={isActive} />}
items={[
{
key: "evaluation",
label: "Evaluation Settings",
children: (
<Row>
<Col style={{ width: "100%" }}>
<Form.Item
label="Use sparse ground truth tracing"
name={["evaluationSettings", "useSparseTracing"]}
valuePropName="checked"
initialValue={false}
tooltip="The evaluation mode can either be `dense`
in case all processes in the volume are annotated in the ground-truth.
If not, use the `sparse` mode."
>
<Checkbox style={{ width: "100%" }} />
</Form.Item>
<Form.Item
label="Max edge length in nm"
name={["evaluationSettings", "maxEdgeLength"]}
tooltip="Ground truth tracings can be densified so that
nodes are at most max_edge_length nm apart.
However, this can also introduce wrong nodes in curved processes."
>
<InputNumber style={{ width: "100%" }} placeholder="None" />
</Form.Item>
<Form.Item
label="Sparse tube threshold in nm"
name={["evaluationSettings", "sparseTubeThresholdInNm"]}
tooltip="Tube threshold for sparse evaluation,
determining if a process is too far from the ground-truth."
>
<InputNumber style={{ width: "100%" }} placeholder="1000" />
</Form.Item>
<Form.Item
label="Sparse minimum merger path length in nm"
name={["evaluationSettings", "minimumMergerPathLengthInNm"]}
tooltip="Minimum ground truth path length of a merger component
to be counted as a relevant merger (for sparse evaluation).
Note, the path length to neighboring nodes of a component is included for this comparison. This optimistic path length
estimation makes sure no relevant mergers are ignored."
>
<InputNumber style={{ width: "100%" }} placeholder="800" />
</Form.Item>
<Form.Item name="useAnnotation" initialValue={true} />
</Col>
</Row>
),
},
]}
activeKey={isActive ? "evaluation" : []}
/>

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see the issue here.

I'd ignore that @cdfhalle

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

);
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect usage of tooltip prop in Form.Item components

The Form.Item component from Ant Design does not support a tooltip prop directly. To display a tooltip for the label, you should wrap the label content with the Tooltip component. This ensures that the tooltip behaves as expected and follows the Ant Design specifications.

Apply this diff to fix the issue:

-                <Form.Item
-                  label="Use sparse ground truth tracing"
-                  name={["evaluationSettings", "useSparseTracing"]}
-                  valuePropName="checked"
-                  initialValue={false}
-                  tooltip="The evaluation mode can either be `dense`
-      in case all processes in the volume are annotated in the ground-truth.
-      If not, use the `sparse` mode."
-                >
+                <Form.Item
+                  label={
+                    <span>
+                      Use sparse ground truth tracing&nbsp;
+                      <Tooltip title="The evaluation mode can either be `dense` in case all processes in the volume are annotated in the ground-truth. If not, use the `sparse` mode.">
+                        <InfoCircleOutlined />
+                      </Tooltip>
+                    </span>
+                  }
+                  name={["evaluationSettings", "useSparseTracing"]}
+                  valuePropName="checked"
+                  initialValue={false}
+                >
                   <Checkbox style={{ width: "100%" }} />
                 </Form.Item>

Apply similar changes to the other Form.Item components with tooltips.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but you are wrong here AI :)
The docs state that there is a tooltip prop

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say to also ignore that @cdfhalle

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


function StartJobForm(props: StartJobFormProps) {
const isBoundingBoxConfigurable = props.isBoundingBoxConfigurable || false;
const isSkeletonSelectable = props.isSkeletonSelectable || false;
const chooseSegmentationLayer = props.chooseSegmentationLayer || false;
const { handleClose, jobName, jobApiCall, fixedSelectedLayer, title, description } = props;
const {
handleClose,
jobName,
jobApiCall,
fixedSelectedLayer,
title,
description,
jobSpecificInputFields,
} = props;
const [form] = Form.useForm();
const rawUserBoundingBoxes = useSelector((state: OxalisState) =>
getUserBoundingBoxesFromState(state),
Expand Down Expand Up @@ -650,6 +727,7 @@ function StartJobForm(props: StartJobFormProps) {
onChangeSelectedBoundingBox={(bBoxId) => form.setFieldsValue({ boundingBoxId: bBoxId })}
value={form.getFieldValue("boundingBoxId")}
/>
{jobSpecificInputFields}
{isSkeletonSelectable && <ShouldUseTreesFormItem />}
{props.showWorkflowYaml ? (
<CollapsibleWorkflowYamlEditor
Expand Down Expand Up @@ -707,6 +785,7 @@ export function NucleiDetectionForm() {
export function NeuronSegmentationForm() {
const dataset = useSelector((state: OxalisState) => state.dataset);
const dispatch = useDispatch();
const [useEvaluation, setUseEvaluation] = React.useState(false);
return (
<StartJobForm
handleClose={() => dispatch(setAIJobModalStateAction("invisible"))}
Expand All @@ -715,18 +794,38 @@ export function NeuronSegmentationForm() {
title="AI Neuron Segmentation"
suggestedDatasetSuffix="with_reconstructed_neurons"
isBoundingBoxConfigurable
jobApiCall={async ({ newDatasetName, selectedLayer: colorLayer, selectedBoundingBox }) => {
if (!selectedBoundingBox) {
jobApiCall={async (
{ newDatasetName, selectedLayer: colorLayer, selectedBoundingBox, annotationId },
form: FormInstance<any>,
) => {
const evaluationSettings = form.getFieldValue("evaluationSettings");
if (!selectedBoundingBox || (useEvaluation && evaluationSettings == null)) {
return;
}

const bbox = computeArrayFromBoundingBox(selectedBoundingBox.boundingBox);
if (!useEvaluation) {
return startNeuronInferralJob(
dataset.owningOrganization,
dataset.name,
colorLayer.name,
bbox,
newDatasetName,
useEvaluation,
);
}
return startNeuronInferralJob(
dataset.owningOrganization,
dataset.name,
colorLayer.name,
bbox,
newDatasetName,
useEvaluation,
annotationId,
evaluationSettings.useSparseTracing,
evaluationSettings.maxEdgeLength,
evaluationSettings.sparseTubeThresholdInNm,
evaluationSettings.minimumMergerPathLengthInNm,
);
}}
description={
Expand All @@ -742,6 +841,9 @@ export function NeuronSegmentationForm() {
</Space>
</>
}
jobSpecificInputFields={
<CollapsibleEvaluationSettings isActive={useEvaluation} setActive={setUseEvaluation} />
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just guessing here: The split merger evaluation requires an annotation to work, doesn't it? And more specific a skeleton annotation is required, isn't it?

If that's so please only set the jobSpecificInputFields to CollapsibleEvaluationSettings if the user currently has an opened annotation with a skeleton annotation.
You can retrieve whether there is a skeleton tracing via:

  const hasSkeletonAnnotation = useSelector((state: OxalisState) => state.tracing.skeleton != null);

/>
);
}
Expand Down