Feat/#160 dicom attr blacklist #163
Conversation
There was a problem hiding this comment.
These are interesting scripts. We can keep them as they are well described.
I haven't seen the exact difference between two of them but haven't looked deep.
Do the scripts only list attributes not already excluded/blacklisted? I think that would be useful, see individual comments.
I think the list is a great start but needs a manual review as per detailed comments.
I think we're handling attrinbute type correctly, right?
Optional improvement would be having all scripts/ be typescript, unless that's difficult.
src/config/dicom/manualBlacklist.ts
Outdated
| 'ConfigurationInformationDescription', | ||
| 'ContainerComponentDescription', | ||
| 'ContentDescription', | ||
| 'ContextGroupExtensionCreatorUID', |
There was a problem hiding this comment.
I think all UIDs are safe? Can we exclude them from the manual blacklist?
There was a problem hiding this comment.
They should be accepted internally (despite being in the blacklist) as per the comments, but yes it's cleaner for me to remove them here :)
src/config/dicom/manualBlacklist.ts
Outdated
| 'PatientEyeMovementCommandCodeSequence', | ||
| 'PatientEyeMovementCommanded', | ||
| 'PatientGantryRelationshipCodeSequence', | ||
| 'PatientIdentityRemoved', |
There was a problem hiding this comment.
This is a breaker if blacklisted (see other logic in code).
Could you do a manual review of the list to avoid these?
src/config/dicom/manualBlacklist.ts
Outdated
| 'PatientEyeMovementCommanded', | ||
| 'PatientGantryRelationshipCodeSequence', | ||
| 'PatientIdentityRemoved', | ||
| 'PatientMotionCorrected', |
There was a problem hiding this comment.
Isn't this a boolean? Can you review all Patient flags for false positives
src/config/dicom/manualBlacklist.ts
Outdated
| 'ReceivingPresentationAddress', | ||
| 'SendingPresentationAddress', | ||
| 'SourcePresentationAddress', | ||
| 'WedgeInContactLength', |
There was a problem hiding this comment.
Not a "contact" in a personal sense here?
src/config/dicom/manualBlacklist.ts
Outdated
| // ======================================================================== | ||
|
|
||
| 'ASLContext', | ||
| 'ASLCrusherDescription', |
There was a problem hiding this comment.
I suggest to remove all "Description" and "Comment" fields from this manual blacklist for consistency, as we're handling Descriptors in a consistent way already (per an actual option laid out in the 3.15 standard):
See deidentifyPS315E.ts:370
src/deidentifyPS315E.ts
Outdated
| ] | ||
| } else if ( | ||
| manualBlacklistSet.has(normalName) && | ||
| vr !== 'UI' && // Don't blacklist UIDs - they have special handling |
There was a problem hiding this comment.
reminder to remove this as we shouldn't put UIs in blacklist - instead, ensure UIs don't get flagged by scripts or blacklisted.
I've tried to automate this and make it repeatable with a bunch of scripts and documentation - but maybe some of this doesn't want to be in the repo?
Either way, I've added the blacklist mechanism and some test coverage. The final list and the rules for generating it, in the scripts, would benefit from second opinions.
Please let me know any concerns