pydicom / deid

best effort anonymization for medical images using python
https://pydicom.github.io/deid/
MIT License
140 stars 43 forks source link

Datetime JITTER Error #122

Closed wetzelj closed 4 years ago

wetzelj commented 4 years ago

Performing JITTER on fields defined as DICOM "DT" datatype throws an Overflow Exception.

Background:

Per the DICOM specification, the DT Datatype is defined as:

A concatenated date-time character string in the format: YYYYMMDDHHMMSS.FFFFFF&ZZXX

The components of this string, from left to right, are YYYY = Year, MM = Month, DD = Day, HH = Hour (range "00" - "23"), MM = Minute (range "00" - "59"), SS = Second (range "00" - "60").

FFFFFF = Fractional Second contains a fractional part of a second as small as 1 millionth of a second (range "000000" - "999999").

&ZZXX is an optional suffix for offset from Coordinated Universal Time (UTC), where & = "+" or "-", and ZZ = Hours and XX = Minutes of offset.

Currently, in deid\utils\actions.py, dateutil.parser.parse is utilized to convert the datetime string to a datetime object. parser.parse does not currently support DICOM DT formatted strings and throws an Overflow Exception.

While I've opened an issue with dateutil for this format, in the meantime, we may want to handle it in actions.py.

Thoughts?

wetzelj commented 4 years ago

We should also think about what happens if there is an exception with parsing... would it be beneficial to set the timestamp to the current timestamp if any exception or error occurs during parsing? I'm thinking that if a user has specified JITTER, they ultimately want the value changed. If something bad happens during parsing, it may be better to default the date to the current date rather than keeping the date unchanged.

vsoch commented 4 years ago

I think this issue would be addressed by #119 - basically doing a get/set based on the datatype. This falls into that bucket of issues. The idea for the parsing also makes sense, we could have a default be the datetime.now().

vsoch commented 4 years ago

@wetzelj I think we fixed this with #119 - please update or close the issue when you are happy!

wetzelj commented 4 years ago

Yes. This was fixed with #119 and covered by test_jitter_timestamp!