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

feat(segmentation export): add new cornerstone3D segmentation export adapter and optimize SEG read #692

Merged
merged 26 commits into from
Jul 28, 2023

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Jul 14, 2023

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

  • Should be able to run the yarn run example segmentationExport and export a SEG

Parsing

  • Test should be done later in OHIF when I create the PR

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • [] "Browser:

@netlify
Copy link

netlify bot commented Jul 14, 2023

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 5ae85c7
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/64c3e0466a43590008c6d42b
😎 Deploy Preview https://deploy-preview-692--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sedghi sedghi changed the title chore(adapters): ability to cache and build ESM modules feat(segmentation export): add new cornerstone3D segmentation export adapter Jul 14, 2023
const volumeImages = volume.convertToCornerstoneImages();
const imagePromises = volumeImages.map(image => image.promise);

await Promise.all(imagePromises).then(images => {
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Member Author

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;
Copy link
Contributor

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?

Copy link
Collaborator

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 = [
Copy link
Collaborator

@wayfarer3130 wayfarer3130 Jul 25, 2023

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.

/**
* _createMultiframeSegmentationFromReferencedImages - description
*
* @param images - An array of the cornerstone image objects.
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Member Author

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.

@@ -1541,3 +1660,109 @@ function getSegmentMetadata(multiframe, seriesInstanceUid) {
data
};
}

function readFromUnpackedChunks(chunks, offset, length) {
Copy link
Collaborator

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.

Copy link
Member Author

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) {
Copy link
Collaborator

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.

Copy link
Member Author

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

Copy link
Collaborator

@wayfarer3130 wayfarer3130 left a 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.

@@ -245,6 +249,10 @@ function metaDataProvider(type, imageId) {
actualFrameDuration: dicomParser.intString(dataSet.string('x00181242')),
};
}

if (type === 'instance') {
return getInstanceModule(imageId, metaDataProvider, instanceModuleNames);
Copy link
Contributor

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.

Copy link
Member Author

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.

@sedghi sedghi merged commit 9e743f5 into main Jul 28, 2023
5 checks passed
wayfarer3130 pushed a commit that referenced this pull request Jul 31, 2023
…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
sedghi pushed a commit that referenced this pull request Jul 31, 2023
…#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 <carsten.walther@mevis.fraunhofer.de>

* 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
@sedghi sedghi changed the title feat(segmentation export): add new cornerstone3D segmentation export adapter feat(segmentation export): add new cornerstone3D segmentation export adapter and optimize SEG read Aug 2, 2023
@sedghi sedghi deleted the feat/dicom-seg-adapter branch August 2, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants