-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
5177f39
make segmentation output layer name for neuron detection configurable
MichaelBuessemeyer 87af329
Merge branch 'master' of github.com:scalableminds/webknossos into con…
MichaelBuessemeyer fa891bd
Merge branch 'master' of github.com:scalableminds/webknossos into con…
MichaelBuessemeyer 5faae78
add OutputSegmentationLayerName to neuron inferral job params
MichaelBuessemeyer cbba404
Merge branch 'master' of github.com:scalableminds/webknossos into con…
MichaelBuessemeyer 612a4a5
add changelog entry
MichaelBuessemeyer 3fbca5d
remove comment
MichaelBuessemeyer efb5083
assert valid output layer + dataset names in backend
fm3 95e5889
git Merge branch 'master' of github.com:scalableminds/webknossos into…
MichaelBuessemeyer 4863778
Merge branch 'configurable-segm-layer-name-for-neuron-job' of github.…
MichaelBuessemeyer edbfc42
more name check
fm3 3d85581
Merge branch 'configurable-segm-layer-name-for-neuron-job' of github.…
fm3 ae20854
Merge branch 'master' of github.com:scalableminds/webknossos into con…
MichaelBuessemeyer 219f908
apply pr feedback
MichaelBuessemeyer 23e38e8
Merge branch 'master' into configurable-segm-layer-name-for-neuron-job
MichaelBuessemeyer 0d81436
Fix previous merge with master
MichaelBuessemeyer 44799d9
Merge branch 'master' of github.com:scalableminds/webknossos into con…
MichaelBuessemeyer a3da4e2
add comment explaining materialize annotation job arguments
MichaelBuessemeyer 07c5638
Merge branch 'master' of github.com:scalableminds/webknossos into con…
MichaelBuessemeyer 7e352fe
Merge branch 'master' into configurable-segm-layer-name-for-neuron-job
MichaelBuessemeyer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
andgetBaseSegmentationName
will return the layer name. ThusvolumeLayerName
isnull
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:
volumeLayerName
will reference the volume layer.baseSegmentationName
will reference the fallback segmentation. The worker job will merge those.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.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.