search

pydicom / deid

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

Bug Fixes and Unit Tests for BLANK & REPLACE Actions & Sequences #242

Closed wetzelj closed 1 year ago

wetzelj commented 1 year ago

Description

  1. Updated BLANK action to function on all VRs.
  2. Added unit tests to ensure BLANK functions on all newly added VRs.
  3. Added unit tests to validate multiple actions on a single field. Added documentation which describes the current interactions.
  4. Corrected issue with REPLACE on numeric VRs (and URI as well).
  5. Added unit tests to validate REPLACE on all (applicable?) VRs. Currently SQ, and the O* VRs are excluded.
  6. Corrected issue preventing actions from executing on fields within sequences.

Related issues:

Checklist

Open questions

  1. For the interaction unit tests and documentation, I simply test and document what the library currently does as of v0.3.1. I would appreciate a closer review of this. As I was building these tests, some of the interactions were not what I expected... but that may just be my mind working a little differently than others! :)
wetzelj commented 1 year ago

The testing is failing due to the changes that were merged into deid-data. The CI testing is still using the old image.

vsoch commented 1 year ago

Old image == old release?

vsoch commented 1 year ago

@wetzelj if you can do a PR to bump the version / changelog over there, I can make a new release!

wetzelj commented 1 year ago

Yeah... sorry about that. I just realized I didn't bump the version of deid-data. Will do it now.

vsoch commented 1 year ago

okay sorry for delay - went into meetings and just coming out! I just merged/released deid-data and did a re-run of the test.

vsoch commented 1 year ago

Woot! passing now, and these tests are fantastic! @wetzelj when you are ready for merge, it shall be so.

wetzelj commented 1 year ago

Thanks! I'm working on a fix for #244 as well that I'd like to get included in this PR (along with some new tests for REPLACE which mirror what I did for BLANK).

vsoch commented 1 year ago

@wetzelj you're amazing!!

wetzelj commented 1 year ago

@vsoch - I think I'm set on what I'd like to get in this PR, but am interested in your feedback if there's anything that you'd like done differently.

wetzelj commented 1 year ago

Adding #243 to this PR as well. Please hold off merging. New commits will arrive tomorrow.

wetzelj commented 1 year ago

@vsoch - This wraps up what I'm intending (plus a little more) for this PR. Once you're good with the inclusions, please go ahead with the merge at your convenience.

vsoch commented 1 year ago

Woohoo! Well done. https://pypi.org/project/deid/0.3.2/

wetzelj commented 1 year ago

Woohoo! Well done. https://pypi.org/project/deid/0.3.2/

Thank you! Speedy merge as always! :)

vsoch commented 1 year ago

I just happen to be up since 6am debugging registry authentication for another OSS project - trying to squeeze in before the workday. Hashtag, maintainer life! :laughing: