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.

Sign up for GitHub

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

Jump to bottom

feat: ROI threshold to consider two volumes for thresholding #325

Merged
merged 21 commits into from
Jan 12, 2023
Merged

feat: ROI threshold to consider two volumes for thresholding #325

merged 21 commits into from
Jan 12, 2023

Conversation

rodrigobasilio2022
Copy link
Contributor

New feature : Add the possibility of use CT volume data in ROI Thresholding via rectangle tool

@netlify
Copy link

netlify bot commented Dec 2, 2022 •
edited
Loading

✅ Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 47d4b22
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63c08b44c181cd0008140237
😎 Deploy Preview https://deploy-preview-325--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 settings.

@sedghi
Copy link
Member

sedghi commented Dec 7, 2022

The CI is failing with this error

**9:57:52 AM: ../core/examples/multiVolumeAPI/index.ts:113:13 - error TS2322: Type '({ volumeActor, preset, }: { volumeActor: any; preset: any; }) => void' is not assignable to type 'VolumeInputCallback'.
9:57:52 AM:   Types of parameters '__0' and 'params' are incompatible.
9:57:52 AM:     Property 'preset' is missing in type '{ volumeActor: vtkVolume; volumeId: string; }' but required in type '{ volumeActor: any; preset: any; }'.
9:57:52 AM: 113             callback: setPetColorMapTransferFunctionForVolumeActor,
9:57:52 AM:                 ~~~~~~~~
9:57:52 AM:   ../core/src/types/IVolumeInput.ts:29:3
9:57:52 AM:     29   callback?: VolumeInputCallback;
9:57:52 AM:          ~~~~~~~~
9:57:52 AM:     The expected type comes from property 'callback' which is declared here on type 'IVolumeInput'
9:57:52 AM: ../core/examples/multiVolumeCanvasToWorld/index.ts:140:7 - error TS2322: Type '({ volumeActor, preset, }: { volumeActor: any; preset: any; }) => void' is not assignable to type 'VolumeInputCallback'.
9:57:52 AM: 140       callback: setPetColorMapTransferFunctionForVolumeActor,
9:57:52 AM:           ~~~~~~~~
9:57:52 AM:   ../core/src/types/IVolumeInput.ts:29:3
9:57:52 AM:     29   callback?: VolumeInputCallback;
9:57:52 AM:          ~~~~~~~~
9:57:52 AM:     The expected type comes from property 'callback' which is declared here on type 'IVolumeInput'
9:57:52 AM: ../core/examples/volumePriorityLoading/index.ts:155:7 - error TS2322: Type '({ volumeActor, preset, }: { volumeActor: any; preset: any; }) => void' is not assignable to type 'VolumeInputCallback'.
9:57:52 AM: 155       callback: setPetColorMapTransferFunctionForVolumeActor,
9:57:52 AM:           ~~~~~~~~
9:57:52 AM:   ../core/src/types/IVolumeInput.ts:29:3
9:57:52 AM:     29   callback?: VolumeInputCallback;
9:57:52 AM:          ~~~~~~~~
9:57:52 AM:     The expected type comes from property 'callback' which is declared here on type 'IVolumeInput'
9:57:52 AM: ../tools/examples/petCt/index.ts:665:9 - error TS2322: Type '({ volumeActor, preset, }: { volumeActor: any; preset: any; }) => void' is not assignable to type 'VolumeInputCallback'.
9:57:52 AM: 665         callback: setPetColorMapTransferFunctionForVolumeActor,
9:57:52 AM:             ~~~~~~~~
9:57:52 AM:   ../core/src/types/IVolumeInput.ts:29:3
9:57:52 AM:     29   callback?: VolumeInputCallback;
9:57:52 AM:          ~~~~~~~~

sedghi
sedghi reviewed Dec 9, 2022
View reviewed changes
packages/core/src/types/IVolumeInput.ts Outdated
@@ -10,6 +10,7 @@ type VolumeInputCallback = (params: {
volumeActor: VolumeActor;
/** unique volume Id in the cache */
volumeId: string;
preset?;
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to put this here, the callback isn't getting a preset, it is using the preset from the surrounding

sedghi
sedghi reviewed Dec 14, 2022
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts
@@ -21,14 +28,17 @@ export type ThresholdRangeOptions = {
*/
function thresholdVolumeByRange(
Copy link
Member

Choose a reason for hiding this comment

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

add documentation to function since you changed the surface parameters

sedghi
sedghi reviewed Dec 14, 2022
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts Outdated
let hits, total;
let range;

// this callback function will test voxels in the finer volume and counts
Copy link
Member

Choose a reason for hiding this comment

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

finer volume -> volume with finer resolution

sedghi
sedghi reviewed Jan 5, 2023
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts
* @param options - the options for thresholding
* @returns segmented volume
*/
function thresholdVolumeByRange(
segmentationVolume: Types.IImageVolume,
referenceVolume: Types.IImageVolume,
thresholdVolumeInformation: ThresholdInformation[],
options: ThresholdRangeOptions
Copy link
Member

Choose a reason for hiding this comment

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

This function should explain in detail what are the overlap types (you have it in the paragraph below) Just add it in the @param options instead of the code body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

sedghi
sedghi reviewed Jan 5, 2023
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts Outdated
Comment on lines 40 to 42
if (!overlapType) {
overlapType = 0; // default is any overlap
}
Copy link
Member

Choose a reason for hiding this comment

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

You can do this in the function argument by saying overlayType=0 to have it default to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
const overlapType = options?.overlapType || 0;

sedghi
sedghi reviewed Jan 5, 2023
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts Outdated
let overlaps, total, range;

/**
* As we are working with volumes of different dimensions, there will be no
Copy link
Member

Choose a reason for hiding this comment

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

The sentence here is wrong as it is not always different dimensions. It should be something like "as there is a chance the volumes might have different dimensions and spacing ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

sedghi
sedghi reviewed Jan 5, 2023
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts Outdated
Comment on lines 173 to 178
const callbackOverlap = ({ value }) => {
total = total + 1;
if (value >= range.lower && value <= range.upper) {
overlaps = overlaps + 1;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this inside testOverlapRange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

sedghi
sedghi reviewed Jan 5, 2023
View reviewed changes
packages/tools/src/utilities/segmentation/thresholdVolumeByRange.ts
Comment on lines +146 to +156
for (let i = 0; i < 2; i++) {
for (let j = 0; j < 2; j++) {
for (let k = 0; k < 2; k++) {
const point = voxelCenter;
point[0] = point[0] + ((i * 2 - 1) * voxelSpacing[0]) / 2;
point[1] = point[1] + ((j * 2 - 1) * voxelSpacing[1]) / 2;
point[2] = point[2] + ((k * 2 - 1) * voxelSpacing[2]) / 2;
voxelCornersWorld.push(point);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a wrong logic. The bounds (corners) to check depends on whether the referenceVoxelSpacing (you need to make that clear that is the other one spacing), is coarser or finer

Copy link
Member

Choose a reason for hiding this comment

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

if the reference (blue) is finer resolution

image

Then going half reference spacing -+ from the center of the grey one doesn't help

Copy link
Member

Choose a reason for hiding this comment

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

But i guess the logic is true the other way around, what do you think

Copy link
Contributor Author

@rodrigobasilio2022 rodrigobasilio2022 Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the logic is correct. If the blue is the reference it should only affect the voxel it is inside.

image

And if it is in the corner of four voxels. It should affect them also

image

The center is not the grey one, but the center of the blue voxel.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you are right, please double check the two scenarios again.

The bounds should always be computed wrt the larger spacing, otherwise the overlaps are computed wrongly

@rodrigobasilio2022
Copy link
Contributor Author

Added spacing check between the segmentation volume and the volumes for range test to correctly define the base volume

@sedghi sedghi changed the title Feat/roi threshold feat: ROI threshold to consider two volumes for thresholding Jan 12, 2023
@sedghi sedghi merged commit 87362af into cornerstonejs:main Jan 12, 2023
wayfarer3130 pushed a commit that referenced this pull request Jan 20, 2023
@richard-viney
fix: exception writing NaN and Infinity values of FD tags (#325)
e86daaa
rodrigobasilio2022 added a commit to RadicalImaging/basilioCornerstone3D that referenced this pull request Jan 21, 2023
@rodrigobasilio2022 @sedghi
feat: ROI threshold to consider two volumes for thresholding (corners…
4f943f7 
…tonejs#325)

* Adding ROI Threshold based on multiple volumes

* Add sliders for petVolume

* Refactoring threshold function parameters

* Working with LPS coords

* Running required checks

* Adding perfusion colormap

* Fix CI error in PR

* Fixing CI function parameter checking error

* Fixing CI error related to api change

* Removing unnecessary extra parameter

* Minor parameter adjustment

* Add test coverType option

* Add some comments

* Convert parameter in optional and set defaul value

* Convert parameter in optional and set defaul value

* Fixing CI errors

* Refactoring and comment threshold functions

* Refactoring code based on PR reviews

* Add spacing check between volumes in roi threshold

* fix build

Co-authored-by: Alireza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sedghi sedghi sedghi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rodrigobasilio2022 @sedghi