blairconrad / dicognito

A library and command line tool for anonymizing DICOM files
MIT License
21 stars 6 forks source link

datetime loading and storage #149

Closed lmdulz closed 1 year ago

lmdulz commented 1 year ago

Hey @blairconrad, I noticed, that you're loading in your datetimeanonymizer.py in both the methods _anonymize_date_and_time and _anonymize_datetime elements of a MultiValue into a list: if isinstance(data_element.value, pydicom.multival.MultiValue): datetimes = list([v for v in data_element.value]) but isn't the value of a MultiValue already a list?

later you store the altered values back concatenating them as a string: new_dates_string = "\\".join(new_dates) data_element.value = new_dates_string (line 98-100)

data_element.value = "\\".join(new_datetimes) (line 121)

but why? Shouldn't they be stored back as a list?

blairconrad commented 1 year ago

Hi, @lmdulz! Maybe you're right. What happens if you change the code?

Possibilities

So many options!

I'll take a gander if I can find some time.

blairconrad commented 1 year ago

While MultiValue objects aren't lists, they do act enough like list that there was no need to copy the individual values into a list, nor to serialize the results. I don't really have enough interest to justify the time it would take to experiment with older versions of pydicom to see if this was not so in the past, so I'll just update the code and move on.

Thanks, @lmdulz.

lmdulz commented 1 year ago

Unfortunately I was mixing up, how single-values are stored accessing them with pydicom and dcmjs (JS) (in dcmjs tag values are stored in a list, either one or multiple in a multivalue) But I saw that you knew that and solved it in your commits - Thank you that you're still taking the time to update that!

blairconrad commented 1 year ago

in dcmjs tag values are stored in a list, either one or multiple in a multivalue

I'm a little sad that pydicom didn't take a similar tack, but they probably had their reasons. Cheerio.