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

OverflowError "VR of 'DS' must be <= 16 characters long" triggered when element is 16 characters long #1632

Closed
fedorov opened this issue Apr 14, 2022 · 6 comments · Fixed by #1633
Labels

Comments

@fedorov
Copy link
Contributor

fedorov commented Apr 14, 2022

Describe the bug

OverflowError triggered while accessing PixelData, which the values compliant with the standard. In the sample referenced in the example below, we have this, which satisfies DS VR:

(0028,0030) DS [.002006091181818\.002006091181818]      #  34, 2 PixelSpacing

But nevertheless the error is triggered while trying to access PixelData:

OverflowError: Values for elements with a VR of 'DS' must be <= 16 characters long, 
but the float provided requires > 16 characters to be accurately represented. Use a 
smaller string, set 'config.settings.reading_validation_mode' to 'WARN' to override 
the length check, or explicitly construct a DS object with 'auto_format' set to True

Expected behavior

OverflowError does not get triggered.

Steps To Reproduce

Follow the steps of this Colab notebook: https://colab.research.google.com/drive/1FcSgjBKazh0YN-jlJYdID0YUTh90CAvZ?usp=sharing

Your environment

module       | version
------       | -------
platform     | Linux-5.4.144+-x86_64-with-Ubuntu-18.04-bionic
Python       | 3.7.13 (default, Mar 16 2022, 17:37:17)  [GCC 7.5.0]
pydicom      | 2.3.0
gdcm         | _module not found_
jpeg_ls      | _module not found_
numpy        | 1.21.5
PIL          | 9.1.0
pylibjpeg    | _module not found_
openjpeg     | _module not found_
libjpeg      | _module not found_

Related issue: imi-bigpicture/wsidicom#49

cc: @DanielaSchacherer @dclunie @hackermd

@pieper
Copy link
Contributor

pieper commented Apr 14, 2022

For reference, a possibly similar issue came up in dcmjs: dcmjs-org/dcmjs#175

@mrbean-bremen
Copy link
Member

I had a quick look, and the problem seems to be that the length is not taken from the original string, but from the string representation, which in this case adds a leading zero... This check has been introduced in pydicom 2.2.0.

mrbean-bremen added a commit to mrbean-bremen/pydicom that referenced this issue Apr 14, 2022
- use str() instead of repr() for validation -
  this uses the original string if available
- fixes pydicom#1632
@mrbean-bremen
Copy link
Member

For reference, a possibly similar issue came up in dcmjs: dcmjs-org/dcmjs#175

This is indeed somewhat similar as it also converted the original value to a normalized representation which is too long for DS. Note that pydicom generally preserves the original string, in this case it just did not do the validation on that string.

@pieper
Copy link
Contributor

pieper commented Apr 14, 2022

The pydicom approach of keeping the original values for DS and other potentially lossy conversions makes good sense and we should see how something similar could be done in javascript.

@mrbean-bremen
Copy link
Member

Yes, this is a simple solution to a non-trivial problem, as it avoids the problems of floating point precision vs notation (or rather sidesteps them...).

mrbean-bremen added a commit to mrbean-bremen/pydicom that referenced this issue Apr 17, 2022
- use str() instead of repr() for validation -
  this uses the original string if available
- fixes pydicom#1632
darcymason pushed a commit that referenced this issue Apr 18, 2022
- use str() instead of repr() for validation -
  this uses the original string if available
- fixes #1632
@fedorov
Copy link
Contributor Author

fedorov commented Apr 18, 2022

Thanks for addressing this so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants