pydicom / deid

best effort anonymization for medical images using python

https://pydicom.github.io/deid/

MIT License

140 stars 43 forks source link

Fix - Multivalued fields in %values lists #176

Closed wetzelj closed 3 years ago

wetzelj commented 3 years ago

Description

Related issues: #174

Checked value before attempting to add it to values list. If the value is MultiValue, use set.Update() instead of set.Add().

Checklist

[x] I have commented my code, particularly in hard-to-understand areas
[x] My changes generate no new warnings
[x] My code follows the style guidelines of this project

Open questions

Questions that require more discussion or to be addressed in future development: None

wetzelj commented 3 years ago

This looks great - a few quick comments / questions! Is there an easy test case for this? You do a fantastic job writing tests so if the answer is no, I'm okay with taking your word that you tested this on some real data and it fixed the bug.

I didn't create a test case for this because I would have had to fabricate multi-occurrence within the test dicom image and given the nature of this change, questioned if it was really worth it. If you want me to, I can come up with something for it...

vsoch commented 3 years ago

Nah I think we are good! If you are going to do some other PRs soon then I'll merge here (but not release) and we can release an updated version when all these changes are in. As always, thanks you @wetzelj !