-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add function to correctly format a DS string #57
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
from math import log10, floor | ||
|
||
from numpy import isfinite | ||
|
||
|
||
def get_ds_string(f: float) -> str: | ||
"""Get a string representation of a float suitable for DS value types | ||
|
||
Returns a string representation of a floating point number that follows | ||
the constraints that apply to decimal strings (DS) value representations. | ||
These include that the string must be a maximum of 16 characters and | ||
contain only digits, and '+', '-', '.' and 'e' characters. | ||
|
||
Parameters | ||
---------- | ||
f: float | ||
Floating point number whose string representation is required | ||
|
||
Returns | ||
------- | ||
str: | ||
String representation of f, following the decimal string constraints | ||
|
||
Raises | ||
------ | ||
ValueError: | ||
If the float is not representable as a decimal string (for example | ||
because it has value ``nan`` or ``inf``) | ||
|
||
""" | ||
if not isfinite(f): | ||
raise ValueError( | ||
"Cannot encode non-finite floats as DICOM decimal strings. " | ||
f"Got {f}" | ||
) | ||
|
||
fstr = str(f) | ||
# In the simple case, the built-in python string representation | ||
# will do | ||
if len(fstr) <= 16: | ||
return fstr | ||
|
||
# Decide whether to use scientific notation | ||
# (follow convention of python's standard float to string conversion) | ||
logf = log10(abs(f)) | ||
use_scientific = logf < -4 or logf >= 13 | ||
|
||
# Characters needed for '-' at start | ||
sign_chars = 1 if f < 0.0 else 0 | ||
|
||
if use_scientific: | ||
# How many chars are taken by the exponent at the end. | ||
# In principle, we could have number where the exponent | ||
# needs three digits represent (bigger than this cannot be | ||
# represented by floats) | ||
if logf >= 100 or logf <= -100: | ||
exp_chars = 5 # e.g. 'e-123' | ||
else: | ||
exp_chars = 4 # e.g. 'e+08' | ||
remaining_chars = 14 - sign_chars - exp_chars | ||
return f'%.{remaining_chars}e' % f | ||
else: | ||
if logf >= 1.0: | ||
# chars remaining for digits after sign, digits left of '.' and '.' | ||
remaining_chars = 14 - sign_chars - int(floor(logf)) | ||
else: | ||
remaining_chars = 14 - sign_chars | ||
return f'%.{remaining_chars}f' % f |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import pytest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CPBridge I assume we no longer need these tests in highdicom, do we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
from highdicom.valuerep import get_ds_string | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"float_val,expected_str", | ||
[ | ||
[1.0, "1.0"], | ||
[0.0, "0.0"], | ||
[-0.0, "-0.0"], | ||
[0.123, "0.123"], | ||
[-0.321, "-0.321"], | ||
[0.00001, "1e-05"], | ||
[3.14159265358979323846, '3.14159265358979'], | ||
[-3.14159265358979323846, '-3.1415926535898'], | ||
[5.3859401928763739403e-7, '5.3859401929e-07'], | ||
[-5.3859401928763739403e-7, '-5.385940193e-07'], | ||
[1.2342534378125532912998323e10, '12342534378.1255'], | ||
[6.40708699858767842501238e13, '6.4070869986e+13'], | ||
[1.7976931348623157e+308, '1.797693135e+308'], | ||
] | ||
) | ||
def test_get_ds_string(float_val: float, expected_str: str): | ||
returned_str = get_ds_string(float_val) | ||
assert len(returned_str) <= 16 | ||
assert expected_str == returned_str | ||
|
||
|
||
@pytest.mark.parametrize( | ||
'float_val', | ||
[float('nan'), float('inf'), float('-nan'), float('-inf')] | ||
) | ||
def test_get_ds_string_invalid(float_val: float): | ||
with pytest.raises(ValueError): | ||
get_ds_string(float_val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make sense for this to be in pydicom instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CPBridge given that this has been merged into pydicom, can we remove this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there's no sense reviewing this PR in its current form