-
Notifications
You must be signed in to change notification settings - Fork 26
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
Changes from 12 commits
b2d9627
ac88d7d
0fa0cfd
3258794
785ddff
bda408e
92b6169
67a55ff
63bade2
870b589
581ed00
2e2d915
9b713b7
d76197b
c5f82f1
55cd8ac
5c7c707
6055346
97cd24c
db3e4d1
4ee1467
213d110
23a95fe
1a7c1eb
282786d
7ca5d48
5f07ad6
8ad0409
9d6572c
e2a045e
e819687
be88173
9be4af9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -223,7 +223,13 @@ class JobController @Inject()( | |
def runInferNeuronsJob(datasetId: String, | ||
layerName: String, | ||
bbox: String, | ||
newDatasetName: String): Action[AnyContent] = | ||
newDatasetName: String, | ||
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 { | ||
|
@@ -245,6 +251,12 @@ class JobController @Inject()( | |
"new_dataset_name" -> newDatasetName, | ||
"layer_name" -> layerName, | ||
MichaelBuessemeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"bbox" -> bbox, | ||
"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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation for evaluation parameters The command arguments need validation to ensure all required evaluation parameters are provided when Add parameter validation using for {
// ... existing validations ...
_ <- Fox.runIf(doSplitMergerEvaluation) {
for {
annotationIdValidated <- annotationId.toFox ?~> "job.inferNeurons.evaluation.missingAnnotationId"
_ <- evalUseSparseTracing.toFox ?~> "job.inferNeurons.evaluation.missingUseSparseTracing"
maxEdgeLength <- evalMaxEdgeLength.toFox.flatMap(v =>
Fox.fromTry(Try(v.toDouble))) ?~> "job.inferNeurons.evaluation.invalidMaxEdgeLength"
sparseTubeThreshold <- evalSparseTubeThresholdNm.toFox.flatMap(v =>
Fox.fromTry(Try(v.toDouble))) ?~> "job.inferNeurons.evaluation.invalidSparseTubeThreshold"
minMergerPathLength <- evalMinMergerPathLengthNm.toFox.flatMap(v =>
Fox.fromTry(Try(v.toDouble))) ?~> "job.inferNeurons.evaluation.invalidMinMergerPathLength"
} yield ()
}
command = JobCommand.infer_neurons
// ... rest of the method Then update the command arguments to use the validated values: commandArgs = Json.obj(
// ... existing fields ...
"do_split_merger_evaluation" -> doSplitMergerEvaluation,
"evaluation_settings" -> (if (doSplitMergerEvaluation) {
Some(Json.obj(
"annotation_id" -> annotationId.get,
"use_sparse_tracing" -> evalUseSparseTracing.get,
"max_edge_length" -> maxEdgeLength,
"sparse_tube_threshold_nm" -> sparseTubeThreshold,
"min_merger_path_length_nm" -> minMergerPathLength
))
} else None)
) |
||
) | ||
job <- jobService.submitJob(command, commandArgs, request.identity, dataset._dataStore) ?~> "job.couldNotRunNeuronInferral" | ||
js <- jobService.publicWrites(job) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -54,6 +54,7 @@ case class Job( | |||||
def datasetId: Option[String] = argAsStringOpt("dataset_id") | ||||||
|
||||||
private def argAsStringOpt(key: String) = (commandArgs \ key).toOption.flatMap(_.asOpt[String]) | ||||||
private def argAsBooleanOpt(key: String) = (commandArgs \ key).toOption.flatMap(_.asOpt[Boolean]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks unformatted: To format the backend code you can use |
||||||
|
||||||
def resultLink(organizationId: String): Option[String] = | ||||||
if (effectiveState != JobState.SUCCESS) None | ||||||
|
@@ -66,6 +67,8 @@ case class Job( | |||||
}.getOrElse(datasetName.map(name => s"datasets/$organizationId/$name/view")) | ||||||
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) => | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 => | ||||||
// Old jobs before the dataset renaming changes returned the output dataset name. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,8 +151,8 @@ features { | |
taskReopenAllowedInSeconds = 30 | ||
allowDeleteDatasets = true | ||
# to enable jobs for local development, use "yarn enable-jobs" to also activate it in the database | ||
jobsEnabled = false | ||
voxelyticsEnabled = false | ||
jobsEnabled = true | ||
voxelyticsEnabled = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be undone before merging |
||
# For new users, the dashboard will show a banner which encourages the user to check out the following dataset. | ||
# If isWkorgInstance == true, `/createExplorative/hybrid/true` is appended to the URL so that a new tracing is opened. | ||
# If isWkorgInstance == false, `/view` is appended to the URL so that it's opened in view mode (since the user might not | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,9 @@ import { | |||||
Switch, | ||||||
type FormInstance, | ||||||
Checkbox, | ||||||
Collapse, | ||||||
Col, | ||||||
InputNumber, | ||||||
} from "antd"; | ||||||
import { | ||||||
startNucleiInferralJob, | ||||||
|
@@ -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; | ||||||
|
@@ -523,11 +527,85 @@ function ShouldUseTreesFormItem() { | |||||
); | ||||||
} | ||||||
|
||||||
cdfhalle marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
function CollapsibleSplitMergersplitMergerEvaluationSettings({ | ||||||
isActive = false, | ||||||
setActive, | ||||||
}: { isActive: boolean; setActive: (active: boolean) => void }) { | ||||||
cdfhalle marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
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 | ||||||
MichaelBuessemeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
layout="horizontal" | ||||||
label="Use sparse ground truth tracing" | ||||||
name={["splitMergerEvaluationSettings", "useSparseTracing"]} | ||||||
valuePropName="checked" | ||||||
initialValue={true} | ||||||
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." | ||||||
MichaelBuessemeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
> | ||||||
<Checkbox style={{ width: "100%" }} /> | ||||||
</Form.Item> | ||||||
<Form.Item | ||||||
label="Max edge length in nm" | ||||||
name={["splitMergerEvaluationSettings", "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" /> | ||||||
MichaelBuessemeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
</Form.Item> | ||||||
<Form.Item | ||||||
label="Sparse tube threshold in nm" | ||||||
name={["splitMergerEvaluationSettings", "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={["splitMergerEvaluationSettings", "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} hidden /> | ||||||
</Col> | ||||||
</Row> | ||||||
), | ||||||
}, | ||||||
]} | ||||||
activeKey={isActive ? "evaluation" : []} | ||||||
/> | ||||||
); | ||||||
} | ||||||
|
||||||
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), | ||||||
|
@@ -650,6 +728,7 @@ function StartJobForm(props: StartJobFormProps) { | |||||
onChangeSelectedBoundingBox={(bBoxId) => form.setFieldsValue({ boundingBoxId: bBoxId })} | ||||||
value={form.getFieldValue("boundingBoxId")} | ||||||
/> | ||||||
{jobSpecificInputFields} | ||||||
{isSkeletonSelectable && <ShouldUseTreesFormItem />} | ||||||
{props.showWorkflowYaml ? ( | ||||||
<CollapsibleWorkflowYamlEditor | ||||||
|
@@ -701,7 +780,9 @@ export function NucleiDetectionForm() { | |||||
} | ||||||
export function NeuronSegmentationForm() { | ||||||
const dataset = useSelector((state: OxalisState) => state.dataset); | ||||||
const hasSkeletonAnnotation = useSelector((state: OxalisState) => state.tracing.skeleton != null); | ||||||
const dispatch = useDispatch(); | ||||||
const [doSplitMergerEvaluation, setDoSplitMergerEvaluation] = React.useState(false); | ||||||
return ( | ||||||
<StartJobForm | ||||||
handleClose={() => dispatch(setAIJobModalStateAction("invisible"))} | ||||||
|
@@ -710,13 +791,40 @@ 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 splitMergerEvaluationSettings = form.getFieldValue("splitMergerEvaluationSettings"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should fix the typing of
Suggested change
|
||||||
if ( | ||||||
!selectedBoundingBox || | ||||||
(doSplitMergerEvaluation && splitMergerEvaluationSettings == null) | ||||||
) { | ||||||
return; | ||||||
} | ||||||
|
||||||
const bbox = computeArrayFromBoundingBox(selectedBoundingBox.boundingBox); | ||||||
return startNeuronInferralJob(dataset.id, colorLayer.name, bbox, newDatasetName); | ||||||
if (!doSplitMergerEvaluation) { | ||||||
return startNeuronInferralJob( | ||||||
dataset.id, | ||||||
colorLayer.name, | ||||||
bbox, | ||||||
newDatasetName, | ||||||
doSplitMergerEvaluation, | ||||||
); | ||||||
} | ||||||
return startNeuronInferralJob( | ||||||
dataset.id, | ||||||
colorLayer.name, | ||||||
bbox, | ||||||
newDatasetName, | ||||||
doSplitMergerEvaluation, | ||||||
annotationId, | ||||||
splitMergerEvaluationSettings.useSparseTracing, | ||||||
splitMergerEvaluationSettings.maxEdgeLength, | ||||||
splitMergerEvaluationSettings.sparseTubeThresholdInNm, | ||||||
splitMergerEvaluationSettings.minimumMergerPathLengthInNm, | ||||||
); | ||||||
}} | ||||||
description={ | ||||||
<> | ||||||
|
@@ -731,6 +839,14 @@ export function NeuronSegmentationForm() { | |||||
</Space> | ||||||
</> | ||||||
} | ||||||
jobSpecificInputFields={ | ||||||
hasSkeletonAnnotation && ( | ||||||
<CollapsibleSplitMergersplitMergerEvaluationSettings | ||||||
isActive={doSplitMergerEvaluation} | ||||||
setActive={setDoSplitMergerEvaluation} | ||||||
/> | ||||||
) | ||||||
} | ||||||
/> | ||||||
); | ||||||
} | ||||||
|
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.
🛠️ Refactor suggestion
Improve parameter type safety and validation
The method signature could be improved in several ways:
doSplitMergerEvaluation
is trueConsider creating a case class for evaluation settings:
Then update the method signature:
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.
This is quite the good suggestion, but the other functions in this file do not have nested parameters. Which I why I would avoid this here as well. Therefore, I would only suggest to fix the type of the numerical parameters to
Option[Double]
:Moreover, their validation is missing below in the code. Please take a look at my suggestion below.
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.
@MichaelBuessemeyer Agreed with your suggestion to use
Option[Double]
for numeric parameters. I'll add a learning about this pattern for future reference.✏️ Learnings added