search

bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
274 stars 157 forks source link

[ENH] Add metadata fields for DeIdentificationMethod/CodeSequence for MRI and PET #1772

Closed CGSchwarzMayo closed 4 weeks ago

CGSchwarzMayo commented 5 months ago

Add tags for DeIdentificationMethod and DeIdentificationMethodCodeSeuence, corresponding to the same-named DICOM tags. These record info on de-identification, including metadata and image content (e.g. de-facing or removing burned-in text)

CGSchwarzMayo commented 5 months ago

This addresses issue #1709

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.22%. Comparing base (0de4c9c) to head (982bc7d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1772 +/- ## ======================================= Coverage 88.22% 88.22% ======================================= Files 16 16 Lines 1393 1393 ======================================= Hits 1229 1229 Misses 164 164 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CGSchwarzMayo commented 5 months ago

I apologize for missing some pieces, and I think I understand, now. Thank you for your careful review and for helping me through this process. I will execute your suggestions and amend the PR with another commit.

effigies commented 5 months ago

No need to apologize! I appreciate the effort you're putting into this.

CGSchwarzMayo commented 5 months ago

Chris,

I committed your suggested diffs as-is. For the tables and sidecars, I deviated a bit in calling both tags optional. My read of DICOM is that if PatientIdentityRemoved is TRUE (which is a tag we don't have in BIDS, and I don't think BIDS really needs it), then at least one of either DeidentificationMethod or DeidentificationCodeSequence is required. Without PatientIdentityRemoved, I would say neither is required, and both are optional. It's not that DeidentificationMethod is recommended if DeidentificationCodeSequence is present, but rather they are of equal status and people can use either. DeidentificationMethod is a single simple string, but there's a greater chance that some downstream software would replace rather than amend the string. DeidentificationCodeSequence better accounts for there being a series of de-identification steps and other software would more likely amend than overwrite it, but it's also a sequence tag and those are unpleasantly messy.

While I was copying from the MRI to the PET table, I noticed the "defacemask" block in the MRI but not PET. I copied this to the PET, since de-facing is also routinely applied to PET now.

I looked for CT-related files, but it appears that CT is not a part of BIDS currently? If CT is ever added, my recommendation is that these elements all be added to CT to match MRI and PET.

Thanks for your help and careful review!

effigies commented 5 months ago

For the tables and sidecars, I deviated a bit in calling both tags optional. My read of DICOM is that if PatientIdentityRemoved is TRUE (which is a tag we don't have in BIDS, and I don't think BIDS really needs it), then at least one of either DeidentificationMethod or DeidentificationCodeSequence is required. Without PatientIdentityRemoved, I would say neither is required, and both are optional.

Sounds good.

While I was copying from the MRI to the PET table, I noticed the "defacemask" block in the MRI but not PET. I copied this to the PET, since de-facing is also routinely applied to PET now.

Could you split that out into a separate PR? I think it will need some additional work to apply properly to PET, and I'd rather not complicate this PR more than necessary.

I looked for CT-related files, but it appears that CT is not a part of BIDS currently? If CT is ever added, my recommendation is that these elements all be added to CT to match MRI and PET.

Correct, and agreed!

CGSchwarzMayo commented 5 months ago

Chris, I attempted to make all the changes you suggested. The automated checks that failed appear to be related to parts of the code that I did not change in this branch, so unrelated to my changes. Thanks for your continuing reviews. Chris

CGSchwarzMayo commented 5 months ago

Thanks Chris (Markiewicz)!

I have a companion PR for dcm2niix at https://github.com/rordenlab/dcm2niix/pull/813, but Chris (Rorden) hasn't commented on it yet. I had initially asked him to add the tags to dcm2niix's .json, and he said to get them added to bids-specification first, so that's how I arrived here.

I don't have any involvement with the other converters.

Thanks, Chris (Schwarz)

effigies commented 2 months ago

I just want to check in on the status here. It looks like there aren't objections, but are we waiting on a PR to bids-examples, or an agreement in principle RE https://github.com/rordenlab/dcm2niix/pull/813?

bendhouseart commented 2 months ago

This looks great as it would greatly speed up my ability to determine if PET images have been defaced or not. Will very happily add this to PET deface as soon as it's merged.