-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Handle large volumes #3235
base: master
Are you sure you want to change the base?
Handle large volumes #3235
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,15 @@ const OBJECT = 'object'; | |
* indiscriminately, but this should be changed). | ||
*/ | ||
class ImageSet { | ||
private rawImages; | ||
|
||
constructor(images) { | ||
if (Array.isArray(images) !== true) { | ||
throw new Error('ImageSet expects an array of images'); | ||
} | ||
|
||
// @property "images" | ||
Object.defineProperty(this, 'images', { | ||
// @property "rawImages" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just set instances to the complete list - instances is used elsewhere to mean all the instance values, whereas images is used for the images for display. |
||
Object.defineProperty(this, 'rawImages', { | ||
enumerable: false, | ||
configurable: false, | ||
writable: false, | ||
|
@@ -32,6 +34,24 @@ class ImageSet { | |
}); | ||
} | ||
|
||
get images(): any { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The general design of the display sets is that they are fixed representations of specific groups of DICOM objects - this mapping allows redefining that, and can cause problems when rendering different image sets. I'd rather have a mapping function produce a new ImageSet entirely rather than having the images be variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't used anywhere here, and I don't see any testing behaviour for it - I think this is a bad idea without careful consideration of what it means for other classes which expect to the representation of display sets to be consistent. |
||
/** | ||
* Extensions can create arbitrary imagesMapper functions to filter/map | ||
* these metadata. For example to only return the first 300 slices | ||
* of a large volume, the below can be run in a command. | ||
* | ||
* const ds = services.DisplaySetService.getDisplaySetByUID(displaySetUID) | ||
* ds.sortByImagePositionPatient() | ||
* ds.setAttribute('imagesMapper', (ds)=> ds.slice(0, 300)) | ||
* displaySetService.setDisplaySetMetadataInvalidated(displaySetUID) | ||
*/ | ||
const imagesMapper = this.getAttribute('imagesMapper'); | ||
if (imagesMapper && imagesMapper instanceof Function) { | ||
return imagesMapper(this.rawImages); | ||
} | ||
return this.rawImages; | ||
} | ||
|
||
getUID() { | ||
return this.uid; | ||
} | ||
|
@@ -48,26 +68,26 @@ class ImageSet { | |
if (typeof attributes === OBJECT && attributes !== null) { | ||
const imageSet = this, | ||
hasOwn = Object.prototype.hasOwnProperty; | ||
for (let attribute in attributes) { | ||
for (const attribute in attributes) { | ||
if (hasOwn.call(attributes, attribute)) { | ||
imageSet[attribute] = attributes[attribute]; | ||
} | ||
} | ||
} | ||
} | ||
|
||
getNumImages = () => this.images.length | ||
getNumImages = () => this.rawImages.length; | ||
|
||
getImage(index) { | ||
return this.images[index]; | ||
return this.rawImages[index]; | ||
} | ||
|
||
sortBy(sortingCallback) { | ||
return this.images.sort(sortingCallback); | ||
return this.rawImages.sort(sortingCallback); | ||
} | ||
|
||
sortByImagePositionPatient() { | ||
const images = this.images; | ||
const images = this.rawImages; | ||
const referenceImagePositionPatient = _getImagePositionPatient(images[0]); | ||
|
||
const refIppVec = new Vector3( | ||
|
@@ -114,11 +134,11 @@ class ImageSet { | |
} | ||
|
||
function _getImagePositionPatient(image) { | ||
return image.getData().metadata.ImagePositionPatient; | ||
return image.ImagePositionPatient; | ||
} | ||
|
||
function _getImageOrientationPatient(image) { | ||
return image.getData().metadata.ImageOrientationPatient; | ||
return image.ImageOrientationPatient; | ||
} | ||
|
||
export default ImageSet; |
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.
This is going to cause rendering issues when items.length ~= n because it is just going to throw out a certain set of instances, which causes the interval between images to be off by a factor of 2. When n << items.length, it works ok,
because the interval becomes nearly consistent again, or if n = items.length / integer value, then one can sub-select evenly. It is possible to write a inconsistent sample viewer, but currently we assume in the configuration that the sample volume is consistent, and don't account for this type of differences.
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 could write a decimator instead - that takes out every other image, or as a general implementation, leaves 1 image, takes k images out, then repeats. Decimators leave consistent spacing.
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 added a decimator to CS3D in the HTJ2K progressive branch if you want one.