pydicom / deid

best effort anonymization for medical images using python

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

MIT License

140 stars 43 forks source link

Pr150 - Bug Fixes and Tests #151

Closed wetzelj closed 3 years ago

wetzelj commented 3 years ago

Description

Related issues: #149

Removed the looping on line 156 in clean.py to correct bug when adding coordinates from recipe file.
Switched the creation of the output folders from a mkdir to makedirs so that the entire tree is created.
Converted self.original.shape to string prior to displaying in warning message. Previously threw an exception.
Added unit tests for the DICOMCleaner clean functionality.

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:

wetzelj commented 3 years ago

I'm not planning to fix the formatting errors, as I think it would be contrary to what we actually want. I believe these are occurring because within this PR, the CI formatting check is still checking using black 19.10b0. The file is formatted using black 20.8b1. I noticed you bumped black to version 20.8b1 in your last commit. Let me know if you want me to do something with this.

wetzelj commented 3 years ago

I couldn't let the formatting go. :)

vsoch commented 3 years ago

Haha ok :) This LGTM - are you ready to merge into the other branch and have everything run together?

wetzelj commented 3 years ago

As you've probably noticed, the loop is back in - I just had to introduce a little check to ensure we had a list. When coordinates are identified from recipe files, they were just returned as a string - not in a list.

Actually, thinking about it a bit more, do you think I should change detect.py (line 208) to return the coordinate from recipe as a list? I think regardless we'll want to retain the list check I introduced as a line of defense.

wetzelj commented 3 years ago

Otherwise, if you're good with it as is, I'd say we're good to merge.

vsoch commented 3 years ago

I think it was working as is - let's leave for now and change if some future discovery of a bug warrants it.