Add/remove keep behavior (Previous PR #199 - 0.2.29 rc) #237
Conversation
…en, changelog update, added PyCharm to gitignore
updated pydicom dependency from 2.1.1 to 2.2.2
* pydicom#197 'remove and keep combo' - added tests that describe situation where remove overrules keep, and also explicitly show that except is using value as substring/regex match * pydicom#197 'remove and keep combo' - added quite straightforward "add fields marked with KEEP to skiplist", also fixed docstring on parser#perform_action and removed unused parameter, fixed typo in random place * pydicom#197 'remove and keep combo' - quotes changed as per code format convention * pydicom#197 'remove and keep combo' - changed keep list to be separate property, updated docs on action order, added two tests for ensuring blank action order * pydicom#197 'remove and keep combo' - changed keep fields collector to be more pythonic * pydicom#197 'remove and keep combo' - more safeguards in keep fields collector, clarified docs upon field that should not be None * pydicom#197 'remove and keep combo' - changelog update, typo fix, version bump
…list of fields being jittered or replaced to prevent them from being removed, tests, extended description of @keep property, changelog update, typo fix, version bump
Co-authored-by: Vanessasaurus <[email protected]>
pydicom#197 'remove and replace/jitter combo' - added once-evaluated list of…
Downstream code expects coordinate definitions to be a comma separated list of values, not a list of ints.
There was a problem hiding this comment.
Just a few tweaks! And @wetzelj your formatting reminds me that we should add pre-commit here - I've been using it (with dev environments for VS Code optionally) for other Python projects and it's 😘 chef's kiss! When we finish up this work I can put this on my queue to do - it makes formatting / linting much easier than what we are doing now.
|
|
||
| # Translate CTP coordinate to the convention we use | ||
| return [xmin, ymin, xmin + width, ymin + height] | ||
| return ",".join([str(i) for i in new_coordinate]) |
There was a problem hiding this comment.
Do we want to return a string here? Coordinates should technically be numbers. I think it would make sense to convert to string only when we need that, or perhaps have the default return as numbers, and have an argument that asks for a string format.
There was a problem hiding this comment.
It depends. :)
Returned as a list of ints, this new function does not work with the existing deid code for coordinates. When I started testing with this, specifying ctpcoordinates ended up with an exception ('int' object has no attribute 'lower') because the output of this function is eventually passed into clean_pixel_data which performs a .lower() on it - expecting that it's a string.
When I received this exception, I targeted the new code for a fix - rather than looking to refactor all of the other places where the code currently generates this string.
There was a problem hiding this comment.
Gotcha, and it looks like it's only used within that utils file:
$ grep -R derive_ctp_coordinate
Binary file deid/config/__pycache__/utils.cpython-39.pyc matches
deid/config/utils.py:def derive_ctp_coordinate(raw):
deid/config/utils.py: coordinate = derive_ctp_coordinate(
deid/config/utils.py: coordinate = derive_ctp_coordinate(
How about let's make it a private function (e.g., just rename to have a single underscore) and call it a day. We mostly don't want a user finding it and using it.
| as they later have to be changed by REPLACE / JITTER | ||
| That allows whitelisting fields from REMOVE ALL/SomeField to change them if needed (i.e. obfuscation) | ||
| """ | ||
| if self._excluded_fields is None: |
There was a problem hiding this comment.
For this I usually prefer a pattern like:
self._excluded_fields = self._excluded_fields or []instead of the nesting.
Committing suggestion. Co-authored-by: Vanessasaurus <[email protected]>
|
Thanks @wetzelj ! |
* add support for translating ctp coordinates * version bump * pin numpy and python 3.7, bump minor version * Add/remove keep behavior (Previous PR #199 - 0.2.29 rc) (#237) * #193 'bump pydicom' - bumped pydicom to 2.2.2, local tests green, changelog update, added PyCharm to gitignore * WIP 197 remove + keep combo should keep (#198) * #197 'remove and keep combo' - added tests that describe situation where remove overrules keep, and also explicitly show that except is using value as substring/regex match * #197 'remove and keep combo' - added quite straightforward "add fields marked with KEEP to skiplist", also fixed docstring on parser#perform_action and removed unused parameter, fixed typo in random place * #197 'remove and keep combo' - quotes changed as per code format convention * #197 'remove and keep combo' - changed keep list to be separate property, updated docs on action order, added two tests for ensuring blank action order * #197 'remove and keep combo' - changed keep fields collector to be more pythonic * #197 'remove and keep combo' - more safeguards in keep fields collector, clarified docs upon field that should not be None * #197 'remove and keep combo' - changelog update, typo fix, version bump * #197 'remove and replace/jitter combo' - added once-evaluated list of fields being jittered or replaced to prevent them from being removed, tests, extended description of @keep property, changelog update, typo fix, version bump * #197 'remove and replace/jitter combo' - run black * Update deid/version.py - agreed to bump to 0.3 * Bug with return of derive_ctp_coordinate Downstream code expects coordinate definitions to be a comma separated list of values, not a list of ints. * Update deid/dicom/parser.py Committing suggestion. * change derive_ctp_coordinates to private function Signed-off-by: vsoch <[email protected]> Co-authored-by: Gleb <[email protected]> Co-authored-by: Vanessasaurus <[email protected]> Signed-off-by: vsoch <[email protected]> Co-authored-by: wetzelj <[email protected]>
Description
Related issues: #197, #198, #204,
Whitelisting was not possible for replacing. Now fields being replaced or jittered are excluded from removal (either REMOVE ALL or REMOVE SomeField).
It is done using separate property and private variable to keep result (lazy eval).
Made clarification regarding KEEP list - those fields are being removed from field list to process, meaning that KEEP SomeField + REPLACE SomeField ... won't work, as kept field is being excluded from list of fields to process for other actions.
Added tests.
Bumped release candidate number and changelog (+small fix for previous change)
Minor docs update.
Checklist
Open questions
Questions that require more discussion or to be addressed in future development: