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 add generate segmentation from dataset #153

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

helghast79
Copy link
Contributor

In some cases cornerstone image loaders deprive _image object from data property, like reported here #144. The added function uses underlying dcmjs functionality but allow users to generate this dataset array themselves and use it to create the segmentation blob.

Dataset array can be filled with cornerstone.metaData.get('instance', imageId) for each image int the series.

Important: each element in dataset needs a _meta property with empty array (_meta=[]) otherwise segmentation will be saved without any metadata

@pieper pieper requested a review from JamesAPetts November 15, 2020 16:44
Copy link
Collaborator

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, but I hope @JamesAPetts has a minute to look.

@JamesAPetts
Copy link
Collaborator

Thanks for this, makes sense to me!

Only things I would suggest is changing the name from generateSegmentationWithDataset to generateSegmentationFromDatasets. "From" implies input rather than an output which "with" suggests, and making datasets plural also demonstrates that it expects and array of datasets.

Also could you expose the call through here in the same way as the current two functions?

You don't need to write an implementation for cornerstoneTools 3, but it'd be good to throw an error if they try to call a v3 method that doesn't exist.

P.S. for readers who stumble upon this thread a year from now:
cornerstone.metaData.get('instance', imageId) is a feature within OHIF using its metadata provider, and just returns the entire naturalized DICOM jason for that imageId. Its not a "standard" feature of any cornerstone image loader.

…on in segmentation object with cornerstone version control/warning
@helghast79
Copy link
Contributor Author

totally agree. Done. Also @pieper & @JamesAPetts, do you think it's better to generate _meta property automatically inside this function if not defined or leave it as it is now? Impact on performance is probably negligible but would facilitate the use of the function since the lack of _meta property will not trigger an error but will produce a segmentation file without metadata.

@pieper
Copy link
Collaborator

pieper commented Nov 16, 2020

It's been a while since I looked, but maybe the _meta would be added by SegmentationDerivation or a superclass?

@helghast79
Copy link
Contributor Author

Well, yes and I thinks that's the problem if the datasets have no _meta property already defined. Segmentation class extends from DerivedPixels class which in turn extends from DerivedDataset. So, the Segmentation constructor call it's superclass parent constructors where _meta property is added by assigning the input datasets _meta property to it.

@@ -57,7 +57,8 @@ function generateSegmentationFromDatasets(
inputLabelmaps3D,
userOptions = {}
) {
//on multiframe images, datasets array contains only 1 element but should work as well
//make sure _meta property is present in every element of datasets with at least an empty array
datasets = datasets.map(ds => ({ ...ds, _meta: ds._meta || [] }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi - thanks for this, I'm trying to get my head back into the details.

Wouldn't we get the same effect more generally if this line where changed to:

_meta: this.referencedDataset._meta || []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Pieper,

Yes, much better. Thanks

Copy link
Collaborator

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👍

@helghast79 helghast79 mentioned this pull request Dec 3, 2020
Closed
@thewildnath
Copy link

Hello! Any update on merging this PR? I am currently trying to use dmcjs to save cornerstone-tools segmentations into a DICOM-SEG file, and am running into the same error that this PR fixes.

@pieper
Copy link
Collaborator

pieper commented Feb 21, 2022

I haven't looked at this in a while - @swederik will you be using this for cs3D segs? Maybe @sedghi should have a look make sure it works with the new code.

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.

4 participants