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

Tag value [undefined] causing unable to write a dicomDict #417

Open
salimkanoun opened this issue Dec 30, 2024 · 9 comments
Open

Tag value [undefined] causing unable to write a dicomDict #417

salimkanoun opened this issue Dec 30, 2024 · 9 comments

Comments

@salimkanoun
Copy link

Hi there,

I have some DICOMs causing some errors during write with DcmJs,

My code is pretty simple, it read a dicom using
const originalDicomDict = dcmjs.data.DicomMessage.readFile(arrayBuffer);

Modify some tags according to a custom logic and then rewrite it using

originalDicomDict.write({ allowInvalidVRLength: true });

However I found some DICOM in which the write() function throws an error :
dcmjs.js?v=61259bcc:4670 Uncaught (in promise) Error: Not a number: undefined at toInt (dcmjs.js?v=61259bcc:4670:15) at WriteBufferStream2.writeUint16 (dcmjs.js?v=61259bcc:4728:46) at dcmjs.js?v=61259bcc:61805:50 at Array.forEach (<anonymous>) at UnsignedShort2.write (dcmjs.js?v=61259bcc:61800:38) at UnsignedShort2.writeBytes (dcmjs.js?v=61259bcc:62884:177) at Tag2.write (dcmjs.js?v=61259bcc:63225:34) at dcmjs.js?v=61259bcc:63494:32 at Array.forEach (<anonymous>) at Function.write (dcmjs.js?v=61259bcc:63487:24)

After investigation, these error is thrown by tags having Value : [undefined] (array length of 1 with an single undefined value), I found this error into the "001021C0" and "00189337" but may also affect other tags eventually.

I'm not sure if it is a read error or a write error, the Value : [undefined] seems a bit curious, maybe a read error generating a Value : [undefined] instead of Value : [] ?

You can find here two instances generating the bug : https://drive.google.com/drive/folders/1rM5YwuEXFSe52YL_mcqmpP_9wQRmuMtL?usp=sharing

in the dicom_error_dcmjs.DCM => see the tag 00180012 => 00189337
in the dicom_error_dcmjs_2.dcm => see the tag 001021C0

@pieper
Copy link
Collaborator

pieper commented Dec 31, 2024

Thanks for reporting this, we want dcmjs to be as robust as possible. That said, I suspect this issue is the result of a combination of some issues with the source data and possibly with the custom logic you are using. For example, the "_2" can't be read with dcmdump from dcmtk.

What would be best if you could contribute a test that demonstrates the issue so it's easy to reproduce.

From my end, I downloaded the data you provided and tested using the this experimental branch that implements a CLI that reads, modifies, and writes a dcm part10 file using dcmjs.

It uses this code:

https://github.com/dcmjs-org/dcmjs-commands/blob/modify/src/index.js#L99-L109

and I can use it to modify tags without error for both the files you provided (and the resulting files do work with dcmdump).

@salimkanoun
Copy link
Author

salimkanoun commented Jan 1, 2025

Hi Steve,

Thank you so much for this guidance, and first of all, best wishes for 2025,

I made a PR here https://github.com/dcmjs-org/dcmjs-commands/pull/4/files in which I added a rewrite command, this command simply read the dicom and re write it identically using DCMJS.

You will see that this command will fail with the 2 previously mentionned dicoms.

The reason is that the dicom dict generated by reading has some tags value Value : [undefined] and this is not accepted by the write function.

The modify does not break because the bug is hidden by the naturalizeDataset / denaturalizeDataset sequence.
I think this line is the one hidding the issue :

naturalDataset[naturalName] = null;

Si it convert undefined to null which make it compatible with the write function.

On my custom code I directly edit the dictionnary without Naturalizing / Denaturalizing it , that's why I have the bug occuring.

I think naturalizing / denaturalizing is not always necessary when you want to edit specific tags and this can have performance impact as naturalization / denaturalization loop all the dataset

Do you think this can be fixed either at the dicomdict generation at reading or on the write function ?

Best regards,

Salim

@pieper
Copy link
Collaborator

pieper commented Jan 1, 2025

Hi and Happy Ney Year Salim!

Ah, I see, yes, it makes sense that the naturalization step would mask the issue.

I think the things to do are 1) make sure the data dict returned by the readDicom method is an accurate reflection of what's in the part 10 file and 2) that writeDicom can robustly handle valid datasets and recreate the original binary data if the data elements are unchanged.

Both of those obviously condition on a level of standardness of the input data (non-standard variants seen widely in the wild we should try to handle, but we can't handle everything).

Given that, do you think you can identify if the problem is with reading or writing and whether the data is correct according to the standard?

Regarding naturalizing/denaturalizing, yes, it should be purely optional. There's definitely a performance and memory penalty so we should make sure everything works if you choose not to use it.

@salimkanoun
Copy link
Author

Dear Steve,

In my opinion the problem is in the reading, having and [undefined] is probably a non sens related to DICOM world.

If we take the definition of 7.1.1 Data Element Fields (https://dicom.nema.org/medical/dicom/current/output/chtml/part05/chapter_7.html)
A Data Element is made up of fields. Three fields are common to all three Data Element structures; these are the Data Element Tag, Value Length, and Value Field.

If we take the DICOM standard about type 2 tags we have
However, it is permissible that if a Value for a Type 2 Data Element is unknown it can be encoded with zero Value Length and no Value

So my understanding is that the value is mendatory in a Data Element, it can be empty but it is defined as empty.

In JS the definition of undefined is
A variable that has not been assigned a value is of type undefined.(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/undefined)

In my opinion if we try to convert the DICOM standard to javascript :

  • Undefined is non assigned value so in that case the DICOM tag should not be in the DICOM at all (as Type 3), if a element is present the value shall be defined.
  • If a tag is in the DICOM it should have a value that can be no value of zero value length. In JS I think null is the best as it specify 1) value is defined 2) it is an empty one

However I'm not expert in low level DICOM aspects, I used to operate DICOM on a higher level, but my feelings is that we should not have undefined after reading a DICOM, if a DICOM tag exist it has a value which can be empty in that case I feel the null primitive more relevant for JS translation

Best regards,

Salim

@salimkanoun
Copy link
Author

by the way another representation compatible with DICOM standard would be also to put a zero length array
[undefined] => []

@pieper
Copy link
Collaborator

pieper commented Jan 3, 2025

Thanks @salimkanoun - did you see #418? It looks like this was a recent regression so hopefully the same fix will close both issues.

@pieper
Copy link
Collaborator

pieper commented Jan 3, 2025

From the research @PantelisGeorgiadis did it seems that the zero length array is the previous behavior and we should restore that.

@salimkanoun
Copy link
Author

Oh yes perfect !

@craigberry1
Copy link
Contributor

Hi @pieper and @salimkanoun 👋

I believe I have a fix for the [undefined] issue described here. It was slightly different #418 but manifested in an almost identical way (Specifically returning single value for empty & fixed tags when function expects an object).

@salimkanoun I used one of your sample DICOM files to verify the fix and unit test but if you have an moment to pull the branch and confirm that would be great, thanks!

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

No branches or pull requests

3 participants