lamyj / odil

Odil is a C++11 library for the DICOM standard
Other
85 stars 21 forks source link

Loss of precision when writing DS #15

Closed lamyj closed 8 years ago

lamyj commented 8 years ago
import odil
data_set = odil.DataSet()
data_set.add(odil.registry.RescaleSlope, [24.5282145946261])
data_set.add(odil.registry.SOPClassUID, ["1.2.3.4"])
data_set.add(odil.registry.SOPInstanceUID, ["1.2.3.4"])
odil.write(data_set, "foo.dcm")

When running odil print foo.dcm (or any other, or open the file with a text editor), we get a loss of precision on Rescale Slope :

SOP Class UID    0008,0016 UI ['1.2.3.4']
SOP Instance UID 0008,0018 UI ['1.2.3.4']
Rescale Slope    0028,1053 DS [24.5282]
lamyj commented 8 years ago

The number of digits should be specified, cf. for example gdcmElement.h

malaterre commented 8 years ago

Great reference :) For more in-depth, have a peak look at http://stackoverflow.com/questions/32631178/writing-ieee-754-1985-double-as-ascii-on-a-limited-16-bytes-string and of course: https://github.com/malaterre/dicomds

lamyj commented 8 years ago

Very thorough analysis! Regarding the runtime of the encoding alone (disabling call to decode and error summing in computeX.c), I get the following results (total and mean per call):

Method Total Mean
compute1.c 4405.588885 s 1.03 µs
compute2.c 1317.886729 s 0.31 µs
compute3.c 1338.936680 s 0.31 µs
compute4.c 12415.340765 s 2.89 µs

For reference, the errors reported in the stackoverflow post are:

Method Total Mean
compute1.c 0.0095729050923877828 2.23×10-12
compute2.c 0.21764383725715469 50.67×10-12
compute3.c 4.050031792674619 942.97×10-12
compute4.c 0.001287056579548422 0.30×10-12

Despite its simplicity, compute3.c can be discarded: compute2.c yields a smaller error with a similar execution time. I'd rather favor error, compactness, and readability (of the result, not of the code) over speed, so compute1.c seems the best version: an error of two parts in a trillion is largely acceptable to me.

malaterre commented 8 years ago

Interesting. Could you reference your compiler (--version, arch, optimization flags) ? As a side note, I would suggest you play with compute1.c first, before merging into your codebase. The resulting string(s) can be rather surprising even for some regular physical measure(s) such as Image Orientation (Patient) and Image Position (Patient).

lamyj commented 8 years ago

Compiler (running on a Mid-2012 Macbook Pro) is

Apple LLVM version 7.3.0 (clang-703.0.31)
Target: x86_64-apple-darwin15.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

I've ran a few checks, including with the value given in the first post which started this bug report, and haven't seen anything fishy: anything specific I should be looking for?

malaterre commented 8 years ago

Re-reading gdcmElement.h it seems I had picked compute4.c at the time...sorry about the noise. Glad compute1.c is working nicely.