WIP 197 remove + keep combo should keep #198

glebsts · 2021-12-16T22:55:56Z

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

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
My code follows the style guidelines of this project
tests added
documentation updated

…ere remove overrules keep, and also explicitly show that except is using value as substring/regex match

…s marked with KEEP to skiplist", also fixed docstring on parser#perform_action and removed unused parameter, fixed typo in random place

vsoch · 2021-12-16T23:04:59Z

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 · 2021-12-17T13:11:28Z

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 · 2021-12-17T16:18:44Z

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.

…ention

…rty, updated docs on action order, added two tests for ensuring blank action order

deid/dicom/parser.py

…re pythonic

…or, clarified docs upon field that should not be None

vsoch

Beautiful! Aside from the one small typo in the docs that I added a suggestion for, this should be good to go. The last bit before merge is to:

bump the version in deid/version.py
update the CHANGELOG.md with a note for this version

docs/_docs/user-docs/recipe-headers.md

glebsts · 2021-12-20T06:01:23Z

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 · 2021-12-20T06:16:02Z

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

vsoch · 2021-12-20T06:17:08Z

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

glebsts · 2021-12-20T06:30:37Z

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

vsoch · 2021-12-20T06:41:43Z

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 · 2021-12-20T06:43:21Z

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

vsoch · 2021-12-20T06:45:22Z

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 · 2021-12-20T06:48:28Z

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 · 2021-12-20T06:49:40Z

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! 🎁🎄

@keep

* #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 Co-authored-by: Gleb <[email protected]> Co-authored-by: Vanessasaurus <[email protected]>

@keep

* 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]>

glebsts added 2 commits December 17, 2021 00:36

#197 'remove and keep combo' - added tests that describe situation wh…

85fbe01

…ere 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 field…

cd51b0a

…s marked with KEEP to skiplist", also fixed docstring on parser#perform_action and removed unused parameter, fixed typo in random place

glebsts added 2 commits December 18, 2021 15:29

#197 'remove and keep combo' - quotes changed as per code format conv…

f7ffb3b

…ention

#197 'remove and keep combo' - changed keep list to be separate prope…

388c5a6

…rty, updated docs on action order, added two tests for ensuring blank action order

vsoch reviewed Dec 19, 2021

View reviewed changes

deid/dicom/parser.py Outdated Show resolved Hide resolved

glebsts added 2 commits December 20, 2021 00:13

#197 'remove and keep combo' - changed keep fields collector to be mo…

e57ee7b

…re pythonic

#197 'remove and keep combo' - more safeguards in keep fields collect…

1319035

…or, clarified docs upon field that should not be None

vsoch approved these changes Dec 20, 2021

View reviewed changes

docs/_docs/user-docs/recipe-headers.md Outdated Show resolved Hide resolved

#197 'remove and keep combo' - changelog update, typo fix, version bump

39ca4e3

vsoch merged commit 1a590bc into pydicom:0.2.29-rc Dec 20, 2021

vsoch mentioned this pull request Dec 20, 2021

action order on same field #197

Closed

glebsts deleted the 197-remove-keep-combo branch December 20, 2021 06:31

glebsts mentioned this pull request Mar 17, 2022

#197 'remove and replace/jitter combo' - added once-evaluated list of… #204

Merged

4 tasks

This was referenced Nov 18, 2022

0.2.29 rc #199

Closed

Add/remove keep behavior (Previous PR #199 - 0.2.29 rc) #237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP 197 remove + keep combo should keep #198

WIP 197 remove + keep combo should keep #198

glebsts commented Dec 16, 2021 •

edited

Loading

vsoch commented Dec 16, 2021

glebsts commented Dec 17, 2021

vsoch commented Dec 17, 2021

vsoch left a comment

glebsts commented Dec 20, 2021

vsoch commented Dec 20, 2021

vsoch commented Dec 20, 2021

glebsts commented Dec 20, 2021

vsoch commented Dec 20, 2021

vsoch commented Dec 20, 2021

vsoch commented Dec 20, 2021

glebsts commented Dec 20, 2021

vsoch commented Dec 20, 2021

WIP 197 remove + keep combo should keep #198

WIP 197 remove + keep combo should keep #198

Conversation

glebsts commented Dec 16, 2021 • edited Loading

Description

Checklist

vsoch commented Dec 16, 2021

glebsts commented Dec 17, 2021

vsoch commented Dec 17, 2021

vsoch left a comment

Choose a reason for hiding this comment

glebsts commented Dec 20, 2021

vsoch commented Dec 20, 2021

vsoch commented Dec 20, 2021

glebsts commented Dec 20, 2021

vsoch commented Dec 20, 2021

vsoch commented Dec 20, 2021

vsoch commented Dec 20, 2021

glebsts commented Dec 20, 2021

vsoch commented Dec 20, 2021

glebsts commented Dec 16, 2021 •

edited

Loading