Add function to correctly format a DS string #57
Conversation
| from numpy import isfinite | ||
|
|
||
|
|
||
| def get_ds_string(f: float) -> str: |
There was a problem hiding this comment.
Wouldn't it make sense for this to be in pydicom instead?
There was a problem hiding this comment.
@pieper I agree, this would ideally go into pydicom. There is an option issue (see pydicom/pydicom#1264). @CPBridge have you considered creating a PR to update pydicom.valuerep.DS?
There was a problem hiding this comment.
@CPBridge given that this has been merged into pydicom, can we remove this module?
There was a problem hiding this comment.
yes, there's no sense reviewing this PR in its current form
| from numpy import isfinite | ||
|
|
||
|
|
||
| def get_ds_string(f: float) -> str: |
There was a problem hiding this comment.
@pieper I agree, this would ideally go into pydicom. There is an option issue (see pydicom/pydicom#1264). @CPBridge have you considered creating a PR to update pydicom.valuerep.DS?
|
I would certainly agree with @hackermd and @pieper that this should be fixed at the pydicom level and this was my first thought also, but given this response in the existing issue in pydicom:
I got the impression that this would not be welcomed. However I suppose I night be reading too much into this and any code that does not force pydicom users to truncate their numbers in a certain way might still be welcomed. I could try adding it as a utility function in pydicom that users of pydicom can choose to use explicitly. Let's park this PR here for now and I will submit a version to pydicom and see where that leads. |
Handling of DS is tricky and can cause really nasty bugs. I understand that we need to workaround compliance issues, but it shouldn't go as far as making it hard for people to actually create valid DICOM in the first place. An approach as proposed in #56 (comment) may help with that. |
|
My approach here is to:
Sound like a good approach? |
Sounds good to me. @darcymason what do you think? Would that be a good compromise? |
Given that it is proposed as an explicit call, I can't see any problem with that. |
|
This functionality has just been merged into pydicom master branch pydicom/pydicom#1334 and should be in the next release of pydicom (looks like this will be 2.2.0). I will adapt this PR to use that function instead |
| @@ -0,0 +1,36 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
@CPBridge I assume we no longer need these tests in highdicom, do we?
There was a problem hiding this comment.
No this PR was originally written with the assumption that the functionality would be in highdicom and I haven't adapted it to at all yet for the fact that the functionality has moved to pydicom. A lot will need to change. I might just create a new PR from scratch, not sure which will end up being easier
|
Closing in favour of #65 |
According to the standard, attributes with the decimal string (DS) value representation must contain a maximum of 16 characters (see here). Unfortunately pydicom does not enforce this, and it looks like there is no movement on @hackermd's open issue to rectify this.
A quick fix was previously implemented in the
PlanePositionSequence, but this was sub-optimal as it truncated (rather than correctly rounding) and did not handle certain cases correctly (e.g. values that python would write in scientific notation when calling the str() method on a float).Furthermore, there are several other instances around the codebase where this fix was not implemented, meaning that invalid DS could be created. For example in NumContentItem, which is used for recording nearly all numeric values in an SR document. I found that this was causing many SRs I created to fail the checks in the dciodvfy tool.
This PR implements a new function that I believe correctly handles all edge cases, and rounds properly. It also checks for nans/infs, which would previously be silently stored as strings despite being invalid values for DS. Also added tests and docs.
I have tried to find all the places in the codebase where DS elements are created and switch to using the new function, but I'm not 100% sure I found that I found them all.