pydicom / deid

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

WIP 197 remove + keep combo should keep #198

Closed glebsts closed 2 years ago

glebsts commented 2 years ago

Description

KEEP someField ignores REMOVE someField by adding it to skip list. ADD someField overrides KEEP someField as discussed under the issue. Added tests. Also added test illustrating that except is working as regex match (documentation to be updated) Related issue: #197

Checklist

vsoch commented 2 years ago

This looks fantastic! It looks like you should run black to fix the formatting issues (black==21.12b0) and then don't forget to increment the version in version.py and add a note to the CHANGELOG.

Do we have except: documented well enough along with the updated use of KEEP?

glebsts commented 2 years ago

No, as I mentioned documentation update it still coming, I'd like to agree on logic change per se. This is reflected in checklist in PR header :)

I looked into failing check, but don't see exact problems there, just that file needs reformatting, so will setup black locally and see what it could give me.

vsoch commented 2 years ago

okay no worries! And yes you can pip install black (I showed the version used in the CI in the comment above) and then run black deid and it should format. Sorry for the extra trouble - black is a bit fussy sometimes, but it's easier to instill a common format for the code.

glebsts commented 2 years ago

I haven't bump version in my last PR, and this PR goes to 0.2.29-RC. So version could be bumped to that one, yeah, just wonder why it is not done yet :)

vsoch commented 2 years ago

oops sorry just forgot about that (it's late and I'm tired!) it looks good to go!

vsoch commented 2 years ago

Done and done! https://pypi.org/project/deid/0.2.28/ Thank you @glebsts !

glebsts commented 2 years ago

Um.. Is 0.2 28 now rebuilt? I'm confused, what's in public repo, real 0.2.28 or 0.2.29rc1?

vsoch commented 2 years ago

Yeah sorry for being confusing! We merged 0.0.28 into master but didn't release on pypi because I was waiting for this fix under that version. So the current version in the repo is 0.0.28.

vsoch commented 2 years ago

ah crap this was merged into your other PR! I totally missed that. Yes we are going to need to do another release.

vsoch commented 2 years ago

So it's up to you how you want to proceed. If you want to do other changes to that branch then go ahead. If you are ready for that branch to PR to master here, then go ahead. I was planning on waiting to release 0.0.28 until we had these changes, but now that will be 0.0.29. Sorry for the confusing-ness, our conversation has happened slowly over time and honestly I just forgot about the plan because I maintain a lot of projects.

glebsts commented 2 years ago

No worries! Thank you :) Yeah, I followed same branch as we agreed last time with pydicom bump, so.. Have some rest! Merry Christmas, and see you next time :)

vsoch commented 2 years ago

Ok, so I’ll do a PR with this branch so we can get it into the mainline, probably later tomorrow. Thank you and merry Christmas! 🎁🎄