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

Make segmentation output layer name for neuron detection configurable #7472

Merged
merged 20 commits into from
Jan 22, 2024

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Dec 4, 2023

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • I suggest testing locally
  • In the applications.conf configure isWkorgInstance=true and jobsEnabled=true
  • Open an annotation of a dataset with at least one color layer.
  • Open the jobs modal via the "AI Analysis" button in the toolbar
  • The default and only available neuron detection job should now have the field to configure the segmentation layer name available.
  • Configure an arbitrary valid name (existing layer names should not be allowed)
  • Open the network tab of the browser console
  • Start the job and inspect whether the correct output segmentation layer name has been sent.

TODOs:

  • Investigate, whether the disallowed layer names should include the name of all color layers (which is afaik currently not the case)
    • The layer names need to be unique.
  • Check which layers are copied over to the new data set by the neuron detection job.
    • Code reads like all layers are copied over and the prediction is added as a new segmentation layer
    • -> Do not allow any existing layer name in the start jobs modal
  • Configure neuron detection job to use new segmentation layer name.

Issues:


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

@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review January 8, 2024 17:41
fm3 and others added 3 commits January 12, 2024 14:30
@fm3
Copy link
Member

fm3 commented Jan 12, 2024

I pushed a commit adding the name checks for the new dataset names and layer names. I want to ask, shold the change of this PR also be added to the infer nuclei job?

@MichaelBuessemeyer
Copy link
Contributor Author

I pushed a commit adding the name checks for the new dataset names and layer names.

Thanks a lot 🙏

I want to ask, should the change of this PR also be added to the infer nuclei job?

I'd say yes. The job cannot be triggered by the UI but is still available in the backend for qualified users in case they send a proper request, right? Therefore, I would also check the names there.

Moreover, I identified the following parameters that are unchecked. Could you please also add the code necessary code here? I failed to get the syntax correct in a reasonable time because I couldn't handle the optional values correctly 😅.

  • runComputeMeshFileJob -> layerName
  • runComputeSegmentIndexFileJob -> layerName
  • runInferNucleiJob -> layerName
  • runInferNeuronsJob -> layerName
  • runExportTiffJob -> layerName & annotationLayerName
  • runMaterializeVolumeAnnotationJob -> fallbackLayerName
  • runFindLargestSegmentIdJob -> layerName

@fm3
Copy link
Member

fm3 commented Jan 12, 2024

I'd say yes. The job cannot be triggered by the UI but is still available in the backend for qualified users in case they send a proper request, right? Therefore, I would also check the names there.

I see. Should the outputSegmentationLayerName be added there as well (and in the worker too)?

…com:scalableminds/webknossos into configurable-segm-layer-name-for-neuron-job
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.

Looks good :) However, see my two comments.

frontend/javascripts/admin/admin_rest_api.ts Outdated Show resolved Hide resolved
Comment on lines +800 to +803
const volumeLayerName =
"fallbackLayer" in segmentationLayer && segmentationLayer.fallbackLayer != null
? getReadableNameOfVolumeLayer(segmentationLayer, tracing)
: null;
Copy link
Member

Choose a reason for hiding this comment

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

so, if no fallbackLayer exists volumeLayerName will simply be null? what will the job do then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, if no fallbackLayer exists volumeLayerName will simply be null? what will the job do then?

In short: Do not merge the fallback data with the volume annotation and simply take the volume data of the segmentation layer given and apply the skeletons of the merger mode.

Longer explanation (tried to make it clear 🙈):
The volumeLayerName is an optional parameter to the materialize job. The materialize job can apply merger mode skeletons as well as merging fallback data with the volume data given by a column annotation.

In case the user has no volume annotation but e.g. wants its skeletons to be applied to a segmentation layer (without any volume annotation), the job just takes the volume layer's segmentation as the segmentation data. => segmentationLayer.fallbackLayer will be null and getBaseSegmentationName will return the layer name. Thus volumeLayerName is null and the job knows to not merge any segmentation data but to only apply the skeleton to the provided segmentation layer's data.

In case there is a volume annotation the fallback layer name still needs to be supplied so it will be merged with the volume annotation before a potential skeleton is applied to merge segments

I hope this is kinda clear. Maybe taking a quick look at the worker job also helps / explains it better: https://github.com/scalableminds/voxelytics/blob/06df1bb2785361b7bfe9abbccb1fb2027d0b2a65/voxelytics/worker/jobs/materialize_volume_annotation_legacy.py#L205

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification! It took me a while to grasp and I hope we can improve the naming and add some comments. I'll try to summarize (correct me if I'm wrong).

There are 3 cases:

  1. There is a fallback segmentation and a volume annotation that is based on that. volumeLayerName will reference the volume layer. baseSegmentationName will reference the fallback segmentation. The worker job will merge those.
  2. There is a segmentation layer and no volume annotation. volumeLayerName will be null (because segmentationLayer.fallbackLayer will be null). baseSegmentationName will reference the only existing segmentation. The worker won't merge anything because volumeLayerName is null.
  3. There is no fallback segmentation and a volume annotation. volumeLayerName will be null (because segmentationLayer.fallbackLayer will be null). baseSegmentationName will reference the volume annotation. As in (2), no merging will be done.

I think, cases 1 + 2 make sense. However, in case 3, I find it quite confusing that the variable name volumeLayerName will be null even though a volume layer exists. Maybe you have an idea for clearing this up a bit. Changing the argument names could get a bit complicated because of the worker and the DB tables, right?
In the simplest case, add a comment about these three cases (like my enumeration above, if it is correct) 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally correct in your enumeration.

I originally designed the first version of this job afaik. I apologize for the confusing naming / confusing arguments passed to the job in case 3. I do not remember whether I had a good reason for this back then.

In the simplest case, add a comment about these three cases (like my enumeration above, if it is correct) 🙂
Ok 👍 I did that

@MichaelBuessemeyer
Copy link
Contributor Author

@fm3 Should someone take a look at your backend changes?

@fm3 fm3 requested a review from frcroth January 17, 2024 10:49
@MichaelBuessemeyer
Copy link
Contributor Author

Should the outputSegmentationLayerName be added there as well (and in the worker too)?

No, I don't think so. The wk UI currently does not support the job anyway. Once it is reenabled, it can be added easily in that same PR.

@fm3 fm3 merged commit 750a607 into master Jan 22, 2024
2 checks passed
@fm3 fm3 deleted the configurable-segm-layer-name-for-neuron-job branch January 22, 2024 14:35
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.

Add form item to enter the name of the output segmentation layer for the inferral jobs
4 participants