search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
287 stars 108 forks source link

Fix/wadors seg creation #360

Open emelalkim opened 12 months ago

emelalkim commented 12 months ago

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

emelalkim commented 12 months ago

@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 commented 12 months ago

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

sedghi commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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

emelalkim commented 7 months ago

@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 commented 7 months ago

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: https://github.com/dcmjs-org/dcmjs/pull/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 commented 7 months ago

@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:

  1. Removal of adapters
  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.
  3. 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:

  1. 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)
  2. 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 commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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

emelalkim commented 6 months ago

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 commented 6 months ago

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