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

Default VR for added private tags #146

Closed
wetzelj opened this issue Sep 11, 2020 · 4 comments
Closed

Default VR for added private tags #146

wetzelj opened this issue Sep 11, 2020 · 4 comments

Comments

@wetzelj
Copy link
Contributor

wetzelj commented Sep 11, 2020

When discussed in #127, we decided to use DICOM VR "UN" (undefined) as the default data type for a private tag. While this seemed to be okay when tested via our current unit tests, this actually causes an exception when calling pydicom FileDataset.save_as to write the output DICOM file due to the fact that the UN datatype is expecting bytes, not a string.

'With tag (1111, 2221) got exception: a bytes-like object is required, not 'str'

Would you be open to defaulting new private tags to one of the string datatypes?

  • LT - Long Text (10240 chars max)
  • LO - Long String (64 chars max)
  • ST - Short Text (1024 chars max)
  • UT - Unlimited Text

If you think this is okay, I'll make the change and add a couple unit tests.

@vsoch
Copy link
Member

vsoch commented Sep 11, 2020

We should likely choose the one that is reasonable and has least potential to spit an error. If we were to do some kind of text, would adding a number spit out an error? Short text seems reasonable to not be too short or too long, and we'd want to discuss if it would work for other types too (or if we should check for the type in python and choose based on that).

@wetzelj
Copy link
Contributor Author

wetzelj commented Sep 11, 2020

Based on what I can tell from some initial evaluation, the ST type will be accepting of most variants we would see. Its accepted positive/negative integers, decimals, and even a string with lengths greater than the limits defined in the standard.

I don't think it's necessarily worth checking in python and dynamically setting the type. If we could definitively determine types, then maybe - but given the way the VRs are defined, we'd have to make some ambiguous decisions.

@vsoch
Copy link
Member

vsoch commented Sep 11, 2020

Ok, let’s go with short text then!

@vsoch
Copy link
Member

vsoch commented Sep 15, 2020

Closed with #148, type changed to short text.

@vsoch vsoch closed this as completed Sep 15, 2020
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

2 participants