Skip to content
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

Datetime JITTER Error #122

Closed
wetzelj opened this issue Mar 9, 2020 · 4 comments
Closed

Datetime JITTER Error #122

wetzelj opened this issue Mar 9, 2020 · 4 comments

Comments

@wetzelj
Copy link
Contributor

wetzelj commented Mar 9, 2020

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
Copy link
Contributor Author

wetzelj commented Mar 9, 2020

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
Copy link
Member

vsoch commented Mar 9, 2020

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
Copy link
Member

vsoch commented Apr 29, 2020

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

@wetzelj
Copy link
Contributor Author

wetzelj commented Apr 30, 2020

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

@wetzelj wetzelj closed this as completed Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants