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

Lots of 'Invalid vr type... ' log messages in dcmjs release 0.29.11 #368

Open
jbocce opened this issue Oct 24, 2023 · 4 comments
Open

Lots of 'Invalid vr type... ' log messages in dcmjs release 0.29.11 #368

jbocce opened this issue Oct 24, 2023 · 4 comments

Comments

@jbocce
Copy link

jbocce commented Oct 24, 2023

Using OHIF...

  1. Open a browser window and open the browser's console window.
  2. Launch the following study in OHIF's dev version https://viewer-dev.ohif.org/viewer/?StudyInstanceUIDs=2.16.840.1.114362.1.11972228.22789312658.616067305.306.2
  3. Notice the many logged error messages in the console. They typically look like the following...
ValueRepresentation.js:262 Invalid vr type xs - using US
value	@	ValueRepresentation.js:262
set	@	ValueRepresentation.js:40
(anonymous)	@	DicomMetaDictionary.js:160
value	@	DicomMetaDictionary.js:117
d	@	index.js:346
(anonymous)	@	index.js:395
(anonymous)	@	index.js:439
Promise.then (async)		
(anonymous)	@	index.js:438
_retrieveSeriesMetadataAsync	@	index.js:437
await in _retrieveSeriesMetadataAsync (async)		
metadata	@	index.js:193
(anonymous)	@	Mode.tsx:67
(anonymous)	@	Mode.tsx:66
(anonymous)	@	Mode.tsx:388
(anonymous)	@	Mode.tsx:400
Ul	@	react-dom.production.min.js:262
t.unstable_runWithPriority	@	scheduler.production.min.js:18
qa	@	react-dom.production.min.js:122
Nl	@	react-dom.production.min.js:261
(anonymous)	@	react-dom.production.min.js:261
x	@	scheduler.production.min.js:16
M.port1.onmessage	@	scheduler.production.min.js:12
  1. Previous versions (namely 0.29.5) did not have this.

Not sure what is causing it. Please advise.

@jbocce
Copy link
Author

jbocce commented Oct 24, 2023

FYI @pieper

@sedghi
Copy link
Collaborator

sedghi commented Oct 24, 2023

Sorry for tagging you Steve, we meant to see if @dmlambo has any comment on this

@pieper
Copy link
Collaborator

pieper commented Oct 24, 2023

Sorry for tagging you Steve, we meant to see if @dmlambo has any comment on this

Thanks, I'm very happy for someone else to investigate this. 👍

For background:

When I click on one of the error message I get to this code:

static createByTypeString(type) {
var vr = VRinstances[type];
if (vr === undefined) {
if (type == "ox") {
// TODO: determine VR based on context (could be 1 byte pixel data)
// https://github.com/dgobbi/vtk-dicom/issues/38
validationLog.error("Invalid vr type", type, "- using OW");
vr = VRinstances["OW"];
} else if (type == "xs") {
validationLog.error("Invalid vr type", type, "- using US");
vr = VRinstances["US"];
} else {
validationLog.error("Invalid vr type", type, "- using UN");
vr = VRinstances["UN"];
}
}
return vr;
}
}

Which reference this issue thread for background:

dgobbi/vtk-dicom#38

But I don't know why the latest changes would trigger more of those cases. Can you maybe bisect between 0.29.5 and 0.29.11 to see exactly which commit leads to the extra logging?

f0dc199

@dmlambo
Copy link
Contributor

dmlambo commented Oct 25, 2023

Ah, yeah that would be my change -- it's a symptom of variable VR types, which I just found out were possible: SmallestImagePixelValue, for example.

What my change introduces is the ability to access values differently than their backing data structure, to allow PN (PersonName) type values to return dicom+json when json stringifying, and formatted strings when writing part10 dcm. As part of that change I obtain the ValueRepresentation class for the VR type and call into a function which can add the required accessors. This actually only happens with PN currently (and may indeed forever stay that way).

The good news is this doesn't affect anything; it's safe to ignore it for the time being. The bad news is the dicom standard is a bit messier than I originally thought. I'll have a look at supressing this accessor step for unknown VRs.

dmlambo added a commit to dmlambo/dcmjs that referenced this issue Oct 25, 2023
When naturalizing a dataset, the ValueRepresentation class used to add
accessors may be ambiguous if the tag has multiple VR types. In this
case we fall back to no accessor. There are only a handful of tags that
fall into this category, and currently only PN has an accessor.
pieper pushed a commit that referenced this issue Nov 2, 2023
When naturalizing a dataset, the ValueRepresentation class used to add
accessors may be ambiguous if the tag has multiple VR types. In this
case we fall back to no accessor. There are only a handful of tags that
fall into this category, and currently only PN has an accessor.
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

4 participants