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(seg): Improve Labelmap Statistics, Interpolation, and Threshold Configuration #1820

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Feb 10, 2025

This PR will:

Labelmap Statistics

  • makes it work on stack viewports too
  • moves the calculation to web worker to not block ui
  • moves it to a utility function instead of a brush strategy (we should be able to calculate stats on anything not just brush instances)

Interpolation

  • makes it work on stack viewports too
  • moves the calculation to web worker to not block ui
  • moves it to a utility function instead of a brush strategy (we should be able to calculate stats on anything not just brush instances)

THRESHOLD -> Strategy specific threshold

Previously, Brush tools using thresholding might have used a generic THRESHOLD property within their strategySpecificConfiguration. This PR introduces a shift towards more explicit and strategy-specific threshold configurations. Instead of a single THRESHOLD property (which is NOT a strategy), you'll now configure thresholds under properties named after the specific strategy itself (e.g., THRESHOLD_INSIDE_CIRCLE, THRESHOLD_INSIDE_SPHERE, THRESHOLD_INSIDE_SPHERE_WITH_ISLAND_REMOVAL).

It is now the app-level concern to make sure threshold is synced between different tool strategies if desired

{
    configuration: {
        activeStrategy: 'THRESHOLD_INSIDE_CIRCLE', 
        strategySpecificConfiguration: {
            THRESHOLD: { // Old generic THRESHOLD property
                threshold: [50, 200], // Example threshold range
            },
        },
    },
});
  • If your activeStrategy is 'THRESHOLD_INSIDE_CIRCLE', rename THRESHOLD to THRESHOLD_INSIDE_CIRCLE.
  • If your activeStrategy is 'THRESHOLD_INSIDE_SPHERE', rename THRESHOLD to THRESHOLD_INSIDE_SPHERE.
  • If your activeStrategy is 'THRESHOLD_INSIDE_SPHERE_WITH_ISLAND_REMOVAL', rename THRESHOLD to THRESHOLD_INSIDE_SPHERE_WITH_ISLAND_REMOVAL.
After (Example - Migrated Configuration):

{
     configuration: {
         activeStrategy: 'THRESHOLD_INSIDE_CIRCLE',
        strategySpecificConfiguration: {
            useCenterSegmentIndex: true,
            THRESHOLD_INSIDE_CIRCLE: { // Renamed to strategy-specific property
                threshold: [50, 200], // Threshold settings remain the same inside
            },
        },
    },
});

sedghi added 13 commits February 6, 2025 16:57
…or brush tools

This commit introduces several improvements to the segmentation strategy configuration:
- Updated strategy-specific configuration to use active strategy as key
- Refactored dynamic threshold and threshold compositions to work with new configuration structure
- Added support for stack and volume viewport strategy data handling
- Simplified strategy data retrieval and configuration management
- Removed deprecated configuration patterns
…provements

- Introduced new `interpolateLabelmap` utility for advanced segmentation interpolation
- Created `getOrCreateSegmentationVolume` helper function
- Updated ITK image conversion utility to support custom scalar data
- Refactored segmentation interpolation strategies
- Simplified segmentation volume creation and management
- Introduced `calculateSpacingBetweenImageIds` utility for precise volume spacing calculation
- Refactored `sortImageIdsAndGetSpacing` to use new spacing calculation method
- Updated `generateVolumePropsFromImageIds` to generate default volume ID
- Exported new spacing calculation utility in index
- Introduced `generateVolumeId` method to create deterministic volume IDs from image IDs
- Added `getImageIdsForVolumeId` to retrieve image IDs associated with a volume
- Implemented `_fnv1aHash` helper method for consistent ID generation
- Created a new cache map `_imageIdsToVolumeIdCache` for tracking image-volume relationships
…generation

- Updated `getOrCreateSegmentationVolume` to handle volume ID retrieval more robustly
- Added support for generating volume IDs from labelmap image IDs
- Simplified volume lookup and creation logic
- Improved type safety with explicit type casting for representation data
Copy link

netlify bot commented Feb 10, 2025

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit ace0183
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/67b4f4b49950a40008f49916
😎 Deploy Preview https://deploy-preview-1820--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 feat/new seg stuff feat(seg): Improve Labelmap Statistics, Interpolation, and Threshold Configuration Feb 10, 2025
@sedghi
Copy link
Member Author

sedghi commented Feb 10, 2025

@wayfarer3130, could you check out peerImport versus import? I'm having trouble getting the labelmapInterpolation example to work without await import. For some reason, running peerImport inside a web worker just isn't working.

@sedghi sedghi requested a review from wayfarer3130 February 10, 2025 16:01
- Added `peerImport` utility function to handle dynamic imports of optional modules
- Updated `getItkImage`, `computeWorker`, and `polySegConverters` to use new peer import mechanism
- Centralized module import strategy to improve modularity and flexibility
* @param imageIds - Array of image IDs
* @returns A deterministic volume ID
*/
public generateVolumeId(imageIds: string[]): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love the idea, however I wonder if the hash length is sufficiently unique that we won't ever generate two hashes containing the same hash code? Would sha1 to 8 digits be better? That is natively encoded and is reasonably fast.
async function sha1(str) {
const enc = new TextEncoder();
const hash = await crypto.subtle.digest('SHA-1', enc.encode(str));
return Array.from(new Uint8Array(hash))
.map(v => v.toString(16).padStart(2, '0'))
.join('');
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, does it need to be order invariant? Should the entries be sorted first?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like crypto apis rely on secure context, so i guess we can't

// @ts-ignore
const { voxelManager } = imageData.get('voxelManager') || {};
if (voxelManager) {
scalarData = voxelManager.getCompleteScalarDataArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be an await version of this to ensure images have loaded and the data is available?

wayfarer3130 and others added 9 commits February 13, 2025 10:52
- Sort image URIs in volume ID generation for consistent hashing
- Enhance scalar data update in VoxelManager to prevent side effects
- Update labelmap interpolation to use active segment index
- Deprecate labelmap interpolation strategy with warning
@@ -49,15 +49,19 @@ export async function peerImport(moduleId) {
}

if (moduleId === '@icr/polyseg-wasm') {
return import('@icr/polyseg-wasm');
return import(
/* webpackChunkName: "icr-polyseg-wasm" */ '@icr/polyseg-wasm'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update with the vite ignore and the separate variable name?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are inside our demo which we don't build and don't ship

@wayfarer3130
Copy link
Collaborator

I'm seeing:
Warning: '@itk-wasm/morphological-contour-interpolation' module not found. Please install it separately.
when I try to run the example. Using the separate name strings doesn't seem to actually work. Is it ok if we just give it a chunk name for now and live with the fact that the binding is hard now? I don't really see a way to fix the workers other than importing them in some hand written wrapper library.

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.

2 participants