dicom
commented
12 years ago Jeff,
First of all, thanks for making such a big effort to improve ruby-dicom's anonymization capabilities! I think you have a lot of good ideas for the future there as well, and Im sure most of them can be included in ruby-dicom.
Regarding the pull request at hand: Generally I prefer to have only one property changed per pull request. -Anonymization script: I think I'll prefer to delay the inclusion of this binary script. From the looks of it we'll get a period of significant changes to the anonymization code in the near future, and I'd like to to delay the introduction of this script until everything has settled down, and then in the end we can discuss the properties of the script in a dedicated issue before we include it. -AuditTrail: I really like the idea of using json for this, and I think we can definitely look into getting rid of the identity file following this. Regarding the file name I would like it to be _ separated as is normal ruby convention, i.e.: audit_trail.rb -Progressbar: Making this optional is an acceptable solution, for the time being. At some time in the future we may revisit this and see if it shall be made a dependency. -Root UID: I like the idea of allowing the user to specify an organization root uid. However, Im not sure about using '555' as the default. Why not use ruby-dicoms root uid instead (the UID constand)? -AuditTrail option: Perhaps using :at is a bit generic and unreadable? Perhaps :trail or :audittrail would be better? -Using uppercase letters in variable names is somewhat un-Ruby-like (e.g. studyUID). Separated by and using lower case would be preferred, e.g. study_uid.
Also, this pull request contains several commits, some of which cancels each other out. It would be really nice if I could get all changes, restricted to the audit trail code as specified above, in a single commit in a new pull request. Would that be possible?
When that gets merged, we can start on your other points, and go through them one by one in separate pull requests.
Thanks, Chris
Chris,
This is just the changes for the jsonauditrail. A couple of things:
1) I use ruby-progressbar in here because from experience, when this takes a while and there is no feedback it's disconcerting. I have made it so if it is available it will be used, if not, then it will still work. 2) The identity file code is still in there. I will remove that in a separate pull request if this works. 3) This includes code to support anonymizing DICOM uids. I could put this in a separate pull-request, but it is pretty much central to the reason I needed the audittrail code.
I would also like to create pull requests for the following functionality:
1) A flag to automatically move DICOM files to a "suspicious_files" directory that are likely to contain PHI (burnt-in data, protocol series, or color 3d reconstructions of a face) 2) More comprehensive defaults for tags to by anonymized. 3) A default list of tags which should simply be removed because their presence leaks information. 4) A limited vocabulary functionality. Users can specify a tag and a JSON file containing a list of allowable values in that tag. If it is not in that list, it would be wiped clean. This is useful for tags that may contain useful information but occasionally are used incorrectly. 5) Removal of identity file code 6) Addition of a ActiveRecord audittrail implementation.
The first four I essentially have done, they just need to be pulled out to make it easy to see. These are all features I have needed when trying to anonymize real DICOM files. Let me know what you think.
Thanks, Jeff