-
Notifications
You must be signed in to change notification settings - Fork 315
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
feat(segmentation export): add new cornerstone3D segmentation export adapter and optimize SEG read #692
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
const volumeImages = volume.convertToCornerstoneImages(); | ||
const imagePromises = volumeImages.map(image => image.promise); | ||
|
||
await Promise.all(imagePromises).then(images => { |
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.
Is PixelData needed to generate the segmentation, or metadata would be enough?
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.
I believe dcmjs only need metadata to create a derived multiframe SEG from the reference
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.
I checked it seems like it needs the referenceDataSet.PixelData
segmentsOnLabelmap3D.add(segmentIndex); | ||
}); | ||
|
||
labelmaps2D[dimensions[2] - z] = labelmap2D; |
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.
Should that be dimensions[2] - 1 - z
?
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.
Is this generating label maps that are frame indexed starting with 1? If so, that fact should be in a comment here and should be noted up top.
|
||
// This is used for gathering all the metadata for export | ||
if (type === 'dataset') { | ||
const types = [ |
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.
Can you externalize this list of types, and add a comment on it? It definitely is NOT all the types, but there doesn't seem to be a safe way to get all the actual types available.
packages/dicomImageLoader/src/imageLoader/wadors/metaData/metaDataProvider.ts
Outdated
Show resolved
Hide resolved
/** | ||
* _createMultiframeSegmentationFromReferencedImages - description | ||
* | ||
* @param images - An array of the cornerstone image objects. |
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.
Could you add a bit more information here on what the images are - I think given the context that the images are the segmentation images, and are thus single bit image objects containing the actual data in the segmentation, but it isn't at all obvious to me externally to this function that the images are just the segmentation single bit images.
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.
Looking at the dcmjs side, these are the original images, and in fact the generation code for the derivation throws away all the pixel data itself, using only the length. I suggest not passing in the actual pixel data, but generating a dummy buffer containing just the byte lengths as required. That way, even the temporary buffer will have the right lengths, as the generated buffer is simply a copy of the derived dataset buffers, and that is usually twice the required length.
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.
we already have the pixelData from the volume. So the cost is negligible compared to creating another temporary buffer etc.
packages/dicomImageLoader/src/imageLoader/wadouri/metaData/metaDataProvider.ts
Outdated
Show resolved
Hide resolved
packages/adapters/src/adapters/Cornerstone3D/Segmentation/generateLabelMaps2DFrom3D.ts
Outdated
Show resolved
Hide resolved
@@ -1541,3 +1660,109 @@ function getSegmentMetadata(multiframe, seriesInstanceUid) { | |||
data | |||
}; | |||
} | |||
|
|||
function readFromUnpackedChunks(chunks, offset, length) { |
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.
Consider using the code from StaticDICOMweb that has all this functionality, exposing it as though it were a single Uint8Array object - that is asyncIterableToBuffer. That allows reading very large buffers and automatically splitting them as required. I think we will need that type of functionality in the future for better handling of streaming results so it would be nice to include. I could add it into dcmjs if that is where we think it belongs.
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.
yeah I think it belongs to dcmjs
}; | ||
} | ||
|
||
function calculateCentroid(imageIdIndexBufferIndex, multiframe) { |
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.
It feels to me like some of these should be added to a helper/utils export rather than being directly included here, as they may well be useful in other segmentation generations in the future, and also that allows explaining what they are doing cleanly.
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.
it is really not a general utility as it is relying on imageIdIndexBufferIndex
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.
Some clean up, but definitely looks good.
…nerstone3D into feat/dicom-seg-adapter
…at/dicom-seg-adapter
@@ -245,6 +249,10 @@ function metaDataProvider(type, imageId) { | |||
actualFrameDuration: dicomParser.intString(dataSet.string('x00181242')), | |||
}; | |||
} | |||
|
|||
if (type === 'instance') { | |||
return getInstanceModule(imageId, metaDataProvider, instanceModuleNames); |
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.
Not sure if it would make sense to use the naturalizeDataset
or namifyDataset
methods of dcmjs
DicomMetaDictionary to get the full metadata of the instance that already match the dcmjs naming.
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.
the problem is that we don't have the instance stored anywhere to naturalize or namify it.
…at/dicom-seg-adapter
…adapter (#692) * chore(adapters): ability to cache and build ESM modules * wip * refactoring * remove bound ijk which is unnecessary since we moved to world * add data set module to the metadata providers * add a new method to segmentation state * add the new parser * work without rle encode * fix demo for export color * add centroid calculations * add trigger event for segmentation load progress * update dcmjs * update api * fix example * remove the promise await * apply review comments * add adapters section to examples * add adapters dep to docs * add eslint rule * example deploy
…#702) * fix(loader): Load colour images correctly when specified Float32Array * feat(streamingVolumeLoader): added IMAGE_VOLUME_LOADING_COMPLETED event (#699) * added IMAGE_VOLUME_LOADING_COMPLETED event * api doc * chore(release): publish [skip ci] * chore(script): add a dev script which should be used for linking (#706) * chore(release): publish [skip ci] * fix(SVGCursorDescriptor): improve CursorSVG typing (#705) * fix: improve CursorSVG typing * chore: update api --------- Co-authored-by: Carsten Walther <[email protected]> * chore(release): publish [skip ci] * feat(segmentation export): add new cornerstone3D segmentation export adapter (#692) * chore(adapters): ability to cache and build ESM modules * wip * refactoring * remove bound ijk which is unnecessary since we moved to world * add data set module to the metadata providers * add a new method to segmentation state * add the new parser * work without rle encode * fix demo for export color * add centroid calculations * add trigger event for segmentation load progress * update dcmjs * update api * fix example * remove the promise await * apply review comments * add adapters section to examples * add adapters dep to docs * add eslint rule * example deploy * chore(release): publish [skip ci] * fix(voi): fix the voi setting when the stack is composed of different orientations (#703) * fix(voi): fix the voi setting when the stack is composed of different orientations * fix the volume conversion to stack voi bug * fix(volumes): a volume should be able to grab data from another cached volume when needed * revert example change * chore(release): publish [skip ci] * feat(voiSync): add optoins to turn of invert sync for voisync (#708) * chore(release): publish [skip ci] * PR requested changes
Context
This PR adds DICOM SEG export for cornerstone3D. It also uses the old importer and optimize and improve upon it for parsing DICOM SEG resulting in 3x faster read time for DICOM SEG.
Changes & Results
modifies the DICOM SEG export in adapters
Testing
Export
yarn run example segmentationExport
and export a SEGParsing
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment