ImagingDataCommons / highdicom

High-level DICOM abstractions for the Python programming language
https://highdicom.readthedocs.io
MIT License
168 stars 35 forks source link

Change person name checks from errors to warnings #154

Closed CPBridge closed 2 years ago

CPBridge commented 2 years ago

I think I was probably a bit over zealous in raising an error when a malformed Person Name is passed. We are finding here that many DICOM files exist "in the wild" with incorrectly formatted patient names, unfortunately. This is now bringing down model inference pipelines when attempting to copy across the name and crashing everything when writing out results...

I suggest a more pragmatic compromise may be to raise a warning instead of an error, as implemented in this PR. @hackermd thoughts?

CPBridge commented 2 years ago

Tagging @delton137

hackermd commented 2 years ago

@CPBridge I ran into problems with this check, too.

Strict checks make sense for construction, but are problematic for parsing. However, we sometimes call constructors via from_dataset(), which can also lead to problems.

CPBridge commented 2 years ago

I did update the tests, no?

Strict checks make sense for construction, but are problematic for parsing.

Right but this is sort of a grey zone on between where we are constructing based on the values found in another file, and there's no way for the code in that function to know whether it was user input or copied from somewhere else.

CPBridge commented 2 years ago

Oh i see it's failing in 3.10, hmm

CPBridge commented 2 years ago

Fixed the tests

seandoyle commented 2 years ago

A further reason why changing the error to a warning is that many anonymization programs (CTP, the old CCDS one, I think a few others) replace the names with strings created with a truncated hash of the original name.