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

fix: 🐛 update invalid VR type for LUTData #343

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bmoix
Copy link

@bmoix bmoix commented Mar 29, 2023

Update the Value Representation (VR) for the LUTData tag. It was defined as lt instead of LT.

The method ValueRepresentation.createByTypeString(type) from ValueRepresentation.js couldn't find the right VR class from the VRInstances object, and it was creating an UnknownValue object. This was causing a crash when reading a file with that tag.

Context

Invalid vr type lt - using UN
(node:28115) UnhandledPromiseRejectionWarning: TypeError: First argument to DataView constructor must be an ArrayBuffer
    at new DataView (<anonymous>)
    at new BufferStream (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:203740)
    at ReadBufferStream._createSuperInternal (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:128580)
    at new ReadBufferStream (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:210562)
    at UnknownValue.writeBytes (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:221335)
    at Tag.write (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:246135)
    at /tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:1105876
    at Array.forEach (<anonymous>)
    at Function.write (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:1105724)
    at SequenceOfItems.writeBytes (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:235261)
(node:28115) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 3)

Tests

All unit tests pass ✔️

Test Suites: 10 passed, 10 total
Tests:       52 passed, 52 total
Snapshots:   0 total
Time:        5.638 s
Ran all test suites.

@pieper
Copy link
Collaborator

pieper commented Mar 29, 2023

Thanks for the report and the proposed fix 👍

Question though: shouldn't the VR be US or OW?

https://dicom.innolitics.com/ciods/nm-image/voi-lut/00283010/00283006

@bmoix
Copy link
Author

bmoix commented Apr 3, 2023

Thanks for taking the time to review this @pieper

Yes, you are right!

In the case that a tag can have more than one VR, what is the best approach? Is using the first one that appears in the tag definition, US in this particular case, good enough? I can see that in dictionary.js you can only assign one VR for each tag.

@pieper
Copy link
Collaborator

pieper commented Apr 6, 2023

That's a good question - I think the right answer is that whoever creates the element needs to provide the _vrMap like is done here.

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