TypeError: Could not convert value to integer without loss

[coreyelowsky]

When trying to read some of my dicom files I get the following error

"TypeError: Could not convert value to integer without loss"

This seems to be because a specific tag had a VR of IS, but the value shown. below is a float?

00000474: 18 00 52 11 49 53 04 00                (0018, 1152) IS Length: 4
0000047c: 34 2e 31 20                            b'4.1 '    

Is there any way I can handle this within pydicom? (e.g. ignore that tag, or truncate it?)

[mrbean-bremen]

I think there is currently no possibility to do this. These values are internally handled as int and the check for a valid int is unconditional. I would actually be in favor of changing this and making it dependent on validation mode, e.g. issue a warning by default in this case, and truncate the float while preserving the original value (in case it is used for writing back).
@darcymason - what do you think?

[coreyelowsky]

Thanks for the response! I think I found a way around this issue via the following code changing the value representation if the value for the tag in question contains a decimal!

if dicom.get_item('Exposure'):
    exp_val = dicom.get_item('Exposure').value
    if '.' in str(exp_val):
        dicom['Exposure'] = pydicom.DataElement(tag='00181152', VR='DS', value=exp_val)

[darcymason]

I would actually be in favor of changing this and making it dependent on validation mode, e.g. issue a warning by default in this case, and truncate the float while preserving the original value (in case it is used for writing back).
@darcymason - what do you think?

I always fall back to the Python "explicit is better than implicit" philosophy, and a warning is not explicit enough, too easy to miss. I could foresee an actual mathematical error being made in some user code calculation due to a truncation like that. I'd be happier with a trappable error or a callback solution.

The OP's solution of changing the VR is a quick workaround, but of course that could be a problem downstream if written out, so needs to be used with caution.

[mrbean-bremen]

I could foresee an actual mathematical error being made in some user code calculation due to a truncation like that.

True. The problem here is that IS is derived from int and cannot take a float - otherwise I would just go with the float after issuing a warning. The problem is that this kind of error seems to happen often enough, and other frameworks may not choke on this. I'm very much for writing correct DICOM, but reading is a different matter, as we all know, and as a user you are often not in a position to correct invalid DICOM coming from a modality.

There is no silver bullet here, of course, and we won't probably do anything for this issue right now (not before version 3 in any case), as a workaround has been found - but in the long run it may make sense to think about this again. The hope that in the near future most DICOM will be valid is futile IMHO, seeing what comes out of brand new modalities in some cases...

[darcymason]

I'm very much for writing correct DICOM, but reading is a different matter, as we all know, and as a user you are often not in a position to correct invalid DICOM coming from a modality.

True...

The problem here is that IS is derived from int and cannot take a float - otherwise I would just go with the float after issuing a warning. The problem is that this kind of error seems to happen often enough, and other frameworks may not choke on this.

You make good points. But I'm tempted to go on a bit of a rant that if other frameworks were not so forgiving, maybe there wouldn't be so many invalid DICOM files. In fact, I did that rant, then deleted the text 😉

In any case, I still believe in safety over convenience. In most cases it is probably quite safe to give a float back, but in rare cases it could cause (really insidious) problems. DICOM could have used a single numeric format, but ints and floats both have their distinct uses. Using int for IS in pydicom is necessary, IMO, to keep the exact math difference that IS gives over float numbers. And we shouldn't expect user code to check whether an actual int or a float came back in an IS data element.

While quite inconvenient for pydicom to error out in these situations (although we now allow setting a float when the exact value is preserved), I think the correct workaround needs the full understanding of the final use of the numbers, which only the user knows for their given situation. Almost certainly fine, but maybe ... just maybe... not.

but in the long run it may make sense to think about this again. The hope that in the near future most DICOM will be valid is futile IMHO, seeing what comes out of brand new modalities in some cases...

Perhaps there is some middle ground where we give the user the option to implicitly type-mutate values like this. (But ... yet another config setting ...) They still could run into problems when using such numbers downstream, but they would at least would be doing it with eyes open.

[mrbean-bremen]

Perhaps there is some middle ground where we give the user the option to implicitly type-mutate values like this. (But ... yet another config setting ...) They still could run into problems when using such numbers downstream, but they would at least would be doing it with eyes open.

Agreed. Doing that unconditionally / by default is probably not a good idea, you have a lot of good points about this. Yet another config setting doesn't sound so great, but that would give the user the possibility to handle such cases without awkward workarounds, and as you say - they would know what they do in this case.

[scaramallion]

I know it's on our collective TODO list anyway, but perhaps the best solution is to make fixing this and other non-conformance reading issues simpler?

Just spit-balling:

def fixer(elem: RawDataElement, *args, **kwargs) -> RawDataElement:
    # Or truncate the value, or whichever
    # Backend change to make RawDataElement mutable needed!
    elem.VR = "DS"
    return elem

dcmread(path, fixes={0x00280010: fixer})

[mrbean-bremen]

Yes, I think that would greatly improve the usability of fixers. We don't really need to make RawDataElement mutable in case that would hit the performance (though I guess that it won't), but that would be a bit less convenient.

[darcymason]

dcmread(path, fixes={0x00280010: fixer})

Yeah, that is pretty interesting.

Another choice which I thought in terms of all settings is a context manager. That could handle multiple situations, settings flags, fixers, etc. and the user controls the scope if it. Although I something like the above would be easier to find in the documentation. Maybe one optional fixers argument and one optional settings argument.

[naterichman]

I know it's been a while, but this is exactly what #1560 was meant to address, I just haven't had any time to finalize it.

[gsmethells]

@darcymason keep in mind what David Clunie said about the topic since he was part of that DICOM committee: http://dclunie.blogspot.com/2008/11/dicom-exposure-attribute-fiasco.html

[darcymason]

@darcymason keep in mind what David Clunie said about the topic since he was part of that DICOM committee: http://dclunie.blogspot.com/2008/11/dicom-exposure-attribute-fiasco.html

I just skimmed through that. I take the point, but also note the reference to the Second-system Effect where a small, elegant and successful system tends to be succeeded by over-engineered, bloated systems... which is one reason why I have tried to avoid dealing with non-standard things in pydicom, to try to keep it small(ish) and elegant (mostly, we hope). But we have tried to allow ways (fixers etc.) to get around the problems, which has worked, albeit not elegantly, for many of the problems that have been encountered.

[mrbean-bremen]

I have tried to avoid dealing with non-standard things in pydicom, to try to keep it small(ish) and elegant

I also agree with that philosopy. Maybe just this one is a special case because it is an unavoidable violation of the standard, and as such actually a common case.

[darcymason]

I'm closing this issue here, but ported @scaramallion's easier fixers access idea to a checklist in the v3.0 release issue.