-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DICOM Redaction Updates #184
Conversation
Note that a0346cc is necessary due to psf/black#3887 |
Should we suppress UserWarnings from pydicom, since we aren't going to do anything about them? In the test files, I get |
I get a pydantic error when I try to use the following rule set override:
Pydantic says |
Aside from pydantic having terrible error messages, we should make it so that a custom rule set has as little required as possible (it can be a separate PR). |
imagedephi/redact/dicom.py
Outdated
self.metadata_redaction_steps[element.tag] = DeleteRule( | ||
key_name=custom_metadata_key, action="delete" | ||
) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be slightly later in the plan: That is, if there is a rule for a specific custom metadata tag, that should take priority over the keep/delete custom metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely move it. But a good point that this raises is that we're relying onpydicom
to get a tag's name, which doesn't work for custom metadata. This means we'll need to also support checking by tag.
Was that the entirety of your rules override file? In that case, the errors are looking for all of the other properties of a Rule Set (name, description, tiff, etc.), since none of it is optional. It would be easy enough to make those things optional as part of this PR. For what its worth, I did end up seeing exactly what fields pydantic was complaining about. There was actually a bug that would have caused that property to not get overridden, so I will address that here. |
This allows more minimal override rulesets.
Move it to later in the build step to give a chance to match a rule.
key_name=custom_metadata_key, action="delete" | ||
) | ||
else: | ||
self.metadata_redaction_steps[element.tag] = KeepRule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this ultimately should have three states, delete custom metadata, keep custom metadata, or do not have auto rules on custom metadata. This could be a future PR, though. Maybe it is a pair of booleans: delete_custom_metadata and keep_custom_metadata which would then make no sense if both true.
Fix #182
Changes
.dcm
extension failed to be redacted.