search

pydicom / deid

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

0.2.29 rc #199

Closed vsoch closed 1 year ago

vsoch commented 2 years ago

Description

@glebsts just opening this so when you have tested and are ready we can merge into master. Definitely no rush, we can do after the holiday! I just want to make sure I don't forget :)

glebsts commented 2 years ago

ack. Have a plan of testing new version in real env this/next week.

vsoch commented 2 years ago

ping @glebsts !

glebsts commented 2 years ago

Hello there, sorry for being late, some focus shift. Re this PR: we tested that on our system and figured out that we also need REPLACE to dominate above REMOVE ALL. Without that it became not so useful. We ended up with switching to own implementation with other tech stack, but if you would accept additional commits in this PR with add replace fields to exceptions from remove all, I would gladly make it.

vsoch commented 2 years ago

yep, that would work, and just make sure to document everything very clearly so there is no doubt about the functionality and order of things.

Pinging @wetzelj to get his feedback on these changes!

wetzelj commented 2 years ago

I've been keeping an eye on this issue/pr and am totally on board with the functionality changes/add. At the moment, we've been working on some other additions to our application that uses pydicom/deid, so we've stuck at 0.2.28, but intend to update to latest (and/or this rc) in the next month or so.

vsoch commented 2 years ago

Fantastic! So @glebsts go ahead if you want to PR your changes to this branch - we can review / discuss and then get that 0.0.29 finalized.

glebsts commented 2 years ago

@vsoch hey, just to clarify - which branch should I should I send PR to (and from which branch better to do it)?

vsoch commented 2 years ago

I think if we want it to extend the work here, it would make sense to PR TO 0.2.29-rc. Is there a better way you have in mind?

glebsts commented 2 years ago

no, just got lost a little bit. Thank you, will work now

glebsts commented 2 years ago

https://github.com/pydicom/deid/pull/204

vsoch commented 2 years ago

okay changes from @glebsts are merged when you have some time @wetzelj ! And looks like I can rename the branch, but it will close the PR! Let's finish review, and I'll rename before merge so it's clear.

vsoch commented 1 year ago

I think we followed up here with a new PR, so its safe to close. Thanks everyone!

wetzelj commented 1 year ago

Hi @vsoch - I think this PR was prematurely closed. When I first saw that this was closed, I did notice that #204 had been merged, but on closer review, #198 and #204 were merged into this PR, #204 wasn't a replacement PR that was merged into master.

Since this was closed and not merged, these changes never made it into master (or subsequent branches).

I am currently testing against PR #234. What do you think about the possibility of merging this into that PR? I still believe that there's definite merit to the changes that were introduced here.

vsoch commented 1 year ago

We sped past this particular release, so if someone wants to re-base and re-open, I'd be happy to review again! It's mostly an interest thing - 7 months with no response I figured it was outdated or not of interest, sorry about that.

wetzelj commented 1 year ago

Sounds good. I'll work on it next week. Is it okay if I base it off of #234 or would you prefer it be based off of master?

vsoch commented 1 year ago

Whichever is easier to test! Both ultimately are going to be rebase off of master, so choose your pick.

wetzelj commented 1 year ago

Great! PR #237 created to merge these changes into #234.