-
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
Make segmentation output layer name for neuron detection configurable #7472
Conversation
…figurable-segm-layer-name-for-neuron-job
…figurable-segm-layer-name-for-neuron-job
…figurable-segm-layer-name-for-neuron-job
… configurable-segm-layer-name-for-neuron-job
…com:scalableminds/webknossos into configurable-segm-layer-name-for-neuron-job
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? |
Thanks a lot 🙏
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 😅.
|
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
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.
Looks good :) However, see my two comments.
const volumeLayerName = | ||
"fallbackLayer" in segmentationLayer && segmentationLayer.fallbackLayer != null | ||
? getReadableNameOfVolumeLayer(segmentationLayer, tracing) | ||
: null; |
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.
so, if no fallbackLayer exists volumeLayerName
will simply be null? what will the job do then?
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.
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
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.
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:
- 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. - There is a segmentation layer and no volume annotation.
volumeLayerName
will be null (becausesegmentationLayer.fallbackLayer
will be null).baseSegmentationName
will reference the only existing segmentation. The worker won't merge anything becausevolumeLayerName
is null. - There is no fallback segmentation and a volume annotation.
volumeLayerName
will be null (becausesegmentationLayer.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) 🙂
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 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
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
@fm3 Should someone take a look at your backend changes? |
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. |
…figurable-segm-layer-name-for-neuron-job
…figurable-segm-layer-name-for-neuron-job
URL of deployed dev instance (used for testing):
Steps to test:
applications.conf
configureisWkorgInstance=true
andjobsEnabled=true
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)