Fix/wadors seg creation #360

emelalkim · 2023-07-20T21:25:12Z

This PR contains the changes required to make dcmjs work with wadors loading mechanism for CornerstoneWadoImageLoader. It keeps the functionality same for wadouri and if the image.data is empty, it retrieves the required data/tags from the wadors metadata.
'build' folder is intentionally added for testing purposed. Should be removed before merge

…create a segmentation Updates generateSegmentation to assume it will retrieve JSON Objects if image.data is empty

It fixes both the dataset retrieve from DICOMweb JSON object by separating file meta and retrieved the sourceImageMetadata from metaDataManager if there is no instance in metadataProvider

…for pixeldata

emelalkim · 2023-07-20T21:28:16Z

@pieper I signed up for the office hours next week but I'd appreciate it if you can look through the code and let us know if you see anything weird. And also I didn't know who to ask for a review, any ideas about who is active?

pieper

Just a couple suggestions, otherwise if it works it looks good to me.

It would be very good to figure out how to make a test though.

pieper · 2023-07-20T21:58:03Z

.gitignore

@@ -7,4 +7,3 @@ node_modules
 yarn-error.log
 yarn.lock
 .DS_Store
-build


Was this intentional? I think we want to keep these build files out of the repo.

yes, that's how we tested it without releasing it. I wanted to keep it till we agree that it's ready to merge so that we can test it again

src/adapters/Cornerstone/Segmentation_4X.js

pieper · 2023-07-20T22:01:40Z

There may be people using this feature in the OHIF community so asking in the office hours makes sense.

sedghi · 2023-07-27T15:54:10Z

@pieper I think we should publish a 1.0 version of dcmjs without the adapters (as we took them outside) so that PRs similar to this happen to our external adapters

pieper · 2023-07-27T16:10:08Z

I think we should publish a 1.0 version of dcmjs without the adapters (as we took them outside) so that PRs similar to this happen to our external adapters

That sounds very good to me. Do you have bandwidth to make a PR?

sedghi · 2023-07-28T12:31:46Z

I will look at it with @wayfarer3130 to see what changes should go to 1.0

emelalkim · 2023-12-12T03:08:00Z

@pieper we are using this as a fork but I wanted to ask my question here as we already have a discussion. I rebased the branch to get the latest changes. The changes that have been done with recent PRs in the patient name are causing some issues.
It started to return dataset.PatientName as [ { Alphabetic: 'BCM-3204-R3TG5-4709' } ] instead of regular text 'BCM-3204-R3TG5-4709'
This didn't seem right to me and I wanted to verify with you that this is the expected behavior before trying to handle it.

Also as there is still development in the adapters part, I assume you decided not to remove those from dcmjs yet. Would you consider merging this? I'll join the office hours to discuss it if you would.

pieper · 2023-12-12T23:25:24Z

It started to return dataset.PatientName as [ { Alphabetic: 'BCM-3204-R3TG5-4709' } ] instead of regular text 'BCM-3204-R3TG5-4709'
This didn't seem right to me and I wanted to verify with you that this is the expected behavior before trying to handle it.

Yes, it should be preserving the PersonName VR type since this change: #364. It should make things better by being lossless. Let us know if it's working for you as expected.

Also as there is still development in the adapters part, I assume you decided not to remove those from dcmjs yet. Would you consider merging this? I'll join the office hours to discuss it if you would.

The adapters part should be dropped at some point, but I have no problem making them work in the meantime.

@sedghi and @wayfarer3130 did you have a chance to look into a release plan?

wayfarer3130 · 2023-12-13T12:41:57Z

@pieper @sedghi - We haven't come up with a release plan for dcmjs to remove the adapters. There are a few things that would be useful to have for that. Would you consider having a second branch for a while to work on a next version release for some length of time - something with breaking changes in it, including:

Removal of adapters
Consistent typing for attributes - make attributes that have VM=1 or 0-1 never be arrays, and with VM>1 always be arrays, and never have the [0] element be top level.
Grouping in well defined ways for private tags (eg group by a tag VR creator value, and put everything underneath that, along with original information)

Maybe:
4. Grouping into tree structured instance, with data attributes shared (for patient/study/series/frame level data), with accessor object for getting single view at any level)
5. Other breaking changes?

I think the plan should be to have a second branch for a half year or so to work on, then when that is stable, rename the current main branch to a v1 branch, and promote the new branch to main.

pieper · 2023-12-13T14:59:41Z

Thanks @wayfarer3130

I think the plan should be to have a second branch for a half year or so to work on, then when that is stable, rename the current main branch to a v1 branch, and promote the new branch to main.

No urgency on my side, just want to be sure we have a way everyone can make progress without stepping on each others' toes.

2. Consistent typing for attributes - make attributes that have VM=1 or 0-1 never be arrays, and with VM>1 always be arrays, and never have the [0] element be top level.

This one has been an issue for a while. Originally the code did this automatically avoid the [0]'s cluttering the code, but it meant a lot of code to check VM which also clutters the application code. Also since the DICOM JSON Model has lists for all Values being consistent with that may be easier for people, but I think pydicom behaves in the way you propose.

Whatever is decided on this one we should take care to be sure it's applied consistently.

Other breaking changes?

Yes, this would be the time if anyone has suggestions.

emelalkim · 2023-12-18T23:36:48Z

Hi @pieper, Ali Reza wants to keep the version in the cornerstone and wants us to contribute there. We'll work on that but it will take some time as there are a lot of changes. Meanwhile, we will have to keep using the fork.
I have updated it to get the latest changes but it actually adds [Object object] in the patient name in the Segmentation DICOM object it writes. I checked the PR for that change and couldn't understand what could be causing it. We are using the dataset to edit/write the segmentation and then datasetToBlob to write the DICOM object. I tried to test this by adding tests but I couldn't, it throws ReferenceError: Blob is not defined error (here is my branch).
Do you have any idea what can be the issue?

wayfarer3130 · 2023-12-19T13:21:53Z

When it writes [Object object] it has ended up converting a JavaScript object into a simple string value on the JavaScript side of things - so probably trying to write a full object definition of patient name into a simple string value.

pieper · 2023-12-20T03:47:34Z

Yes, this may need to be handled as a special case now. Perhaps by doing JSON.stringify on any PN?

emelalkim · 2023-12-20T05:33:39Z

Thank you for your answers @wayfarer3130, @pieper. Shouldn't the part generating the DICOM 10 object/blob handle the change in the format of the PatientName to PN? Where should it be handled? DicomMetaDictionary.denaturalizeDataset()?

emelalkim · 2023-12-21T06:12:24Z

@pieper, got it. There is no issues with [Object object]

emelalkim added 23 commits July 15, 2023 13:36

add _createSegFromJSONObjects method to use DICOMweb JSON Objects to …

9915f84

…create a segmentation Updates generateSegmentation to assume it will retrieve JSON Objects if image.data is empty

add build for debugging

0854eba

fix dcmjs reference

348ddd6

update builds

eddc951

update generate segmentation to handle cornerstone metadata objects

e2b8c69

update builds

b198472

fix classname

bc14414

temporary _meta fill in

0d9eaee

fix dataset and sourceImageMetadata retrieve

94b3808

It fixes both the dataset retrieve from DICOMweb JSON object by separating file meta and retrieved the sourceImageMetadata from metaDataManager if there is no instance in metadataProvider

update builds

433f221

update remaining sourceImageMetadata retrievals

63df7b1

update builds

d29d71c

remove builds

98d92d3

add builds

d79fbb1

fix metadata retrieval

32b3b1e

update builds

1694687

fix other modules

3d4b1fc

update sopcommonmodule retrieval

f90bbfb

update builds

bdd72b5

remove unnecessary ImageOrientationPatient manupulation and adds log …

9602624

…for pixeldata

update builds

ce3e86b

remove log

9145102

update build

4f843c4

pieper requested changes Jul 20, 2023

View reviewed changes

refactor generateSegmentation to remove duplicate code

cdf7f04

emelalkim added 2 commits December 8, 2023 19:43

Merge branch 'master' into fix/wadorsSegCreation

a1ffae5

update builds after rebase

4107308

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/wadors seg creation #360

Fix/wadors seg creation #360

emelalkim commented Jul 20, 2023

emelalkim commented Jul 20, 2023

pieper left a comment

pieper Jul 20, 2023

emelalkim Jul 21, 2023

pieper commented Jul 20, 2023

sedghi commented Jul 27, 2023

pieper commented Jul 27, 2023

sedghi commented Jul 28, 2023

emelalkim commented Dec 12, 2023

pieper commented Dec 12, 2023

wayfarer3130 commented Dec 13, 2023

pieper commented Dec 13, 2023

emelalkim commented Dec 18, 2023

wayfarer3130 commented Dec 19, 2023

pieper commented Dec 20, 2023

emelalkim commented Dec 20, 2023

emelalkim commented Dec 21, 2023

Fix/wadors seg creation #360

Are you sure you want to change the base?

Fix/wadors seg creation #360

Conversation

emelalkim commented Jul 20, 2023

emelalkim commented Jul 20, 2023

pieper left a comment

Choose a reason for hiding this comment

pieper Jul 20, 2023

Choose a reason for hiding this comment

emelalkim Jul 21, 2023

Choose a reason for hiding this comment

pieper commented Jul 20, 2023

sedghi commented Jul 27, 2023

pieper commented Jul 27, 2023

sedghi commented Jul 28, 2023

emelalkim commented Dec 12, 2023

pieper commented Dec 12, 2023

wayfarer3130 commented Dec 13, 2023

pieper commented Dec 13, 2023

emelalkim commented Dec 18, 2023

wayfarer3130 commented Dec 19, 2023

pieper commented Dec 20, 2023

emelalkim commented Dec 20, 2023

emelalkim commented Dec 21, 2023