search

pydicom / deid

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

Allow other vr types for jitter #180

Closed vsoch closed 3 years ago

vsoch commented 3 years ago

Description

@wetzelj I'm not sure if this is what you had in mind? The idea is that if the type doesn't match, we don't know the format string so we try them all until we find one that works.

Related issues: #175

I'm not sure if we need to parse the field differently (e.g., if it's a list). Could you show me an example if this is the case? I also accidentally ran black because it's sort of second nature, sorry for the extra changes to parse through!

wetzelj commented 3 years ago

Hey @vsoch - I'm running out of time for the day and I didn't want to leave this hanging, but don't have anything ready to cleanly add to the pull request. I've committed adjustments to the ctbrain1.dcm file as well as a new unit test to a new branch on my fork (other-vr-jitter). The unit test currently fails, but its due to the fact that the field that I placed the "other VR date" in is a private tag - and jitter_timestamp isn't happy with it, when jitter_timestamp is called, field.element.keyword is passed in as the field - and it's blank for the private field.

What you've implemented is definitely what I had in mind.

Have a great weekend!

vsoch commented 3 years ago

Ah, no worries! We can pick this up when you have time. Does the other PR look good (and would you like me to merge and release, or wait for this fix too?)

wetzelj commented 3 years ago

The other PR looks good, but if you don't mind, let's hold the release for this one too... I'll get back to it early next week.

wetzelj commented 3 years ago

Hey @vsoch - Check out the latest commit on my fork (other-vr-jitter). In building the unit test for this, I discovered that Jitter really didn't work for private tags since the jitter functionality was relying on the keyword to get the field. I went a bit out on a limb fixing this - thought that really it would be better to update the dicom/fields in one place, so I refactored jitter_timestamp to just return the new value, so that we could then just use replace_field() to set the new value. If you don't like this approach, I'm totally fine scrapping it, but if you do think it's okay, I'll submit a PR.

vsoch commented 3 years ago

@wetzelj yes this is so much better! I was thinking about how the design for this function is different from all others (e.g., it interacts with the dicom directly) and that's an artifact of how deid was originally designed. If you want to PR with this update I'll review asap and totally okay to replace this branch of work.

vsoch commented 3 years ago

Superseded by #181

wetzelj commented 3 years ago

@vsoch - I think this needed to be merged into master - not closed. #181 was based on what you started here.

vsoch commented 3 years ago

oh my mistake - I thought you had included these commits?

vsoch commented 3 years ago

@wetzelj could you look at why the test is failing? I think you made some updates that I'm not privy to, I wasn't expecting it to fail!

wetzelj commented 3 years ago

Yep. Looking into it.

wetzelj commented 3 years ago

This was all my bad. Sorry.

There were some conflicts between the fix for #175 and #178. Since I hadn't merged the fix for #178 in, when you merged to master, the conflicts caused the error.

Again... sorry for the issues.

vsoch commented 3 years ago

No need to apologize - you really didn't do anything wrong! These kinds of things happen with multiple changes going on at once. I'm very grateful that you fixed everything up, and we now have a release!