pydicom / deid

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

Recipe Error - REPLACE Rule After JITTER Does Not Replace #173

Closed wetzelj closed 3 years ago

wetzelj commented 3 years ago

When testing the ability to use variable fields in recipes, we discovered that using the REPLACE keyword in a recipe for values did not work. Both samples below should yield the same results to the file header.

Failed:

JITTER endsWith:Date -5814
REPLACE StudyDate var:myOtherDate

Successful:

JITTER endsWith:Date -5814
REMOVE StudyDate
ADD StudyDate var:myOtherDate

The real issue here: When a REPLACE rule is executed following a previous JITTER rule, the replace is not executed. When a REPLACE rule is executed independently, it does function appropriately.

Failing unit tests have been committed to the wetzelj/deid INFOR-646 branch to demonstrate this issue. I have not yet done investigation of cause/fix.

vsoch commented 3 years ago

@wetzelj I can take a look this weekend if you don't get to it.

wetzelj commented 3 years ago

@vsoch - I looked into this a little bit today and I think I'm going to need your help on this one whenever you have a few moments. I can see what's going on, but I don't understand it.

In a successful REPLACE action (I was using test_replace_with_constant to see this), when the value is replaced in parser.py - add_field() - on line 397, the statement updates the value of the item in self.fields as well as self.dicom.

Before executing line 397: image After executing line 397: image

In the error scenario (using test_jitter_replace_compounding), when the value is replaced in parser.py, this same line of code updates the value in self.fields, but fails to update self.dicom. Before executing line 397: image After executing line 397: image

Later on, when we perform the pydicom.save_dicom() action in header.py (line 154), we're saving parser.dicom. Since the value in parser.dicom was never updated, the header is saved with the old value.

vsoch commented 3 years ago

I think that's the issue - when we run jitter_timestamp instead of just saving the value to self.dicom, we also need to update self.fields (which isn't done).

vsoch commented 3 years ago

okay I have a fix! I'm going to have dinner but I'll PR after.

vsoch commented 3 years ago

Fixed by #178 .

@wetzelj time for a release?

wetzelj commented 3 years ago

Yep! I think we're good to go.

wetzelj commented 3 years ago

Fixed with 0.2.25