highdicom: Highdicom allows incorrectly formatted "person name" (PN) attributes

There are a few places where the highdicom API requests str parameters and directly encodes them as attributes with value representation PN (person name). This includes but (but may not be limited to) the content_creator_name parameter of the segmentation SOP constructor, and the verifying_observer_name parameter of the EnhancedSR, ComprehensiveSR, and Comprehensive3DSR constructors.

The format of a PN attribute is quite specific - you can’t just enter free text here. See the PN entry in this table. Briefly, for human names there are five fields (family name, given name, middle name, name prefix, name suffix) that should be separated by caret characters (^). See also the examples in the standard.

Unfortunately, no attempt is made at the pydicom level to enforce or check correct formatting. This propagates to highdicom. Therefore there is no checking or enforcement on these in highdicom, nor any documentation that there is even a format that should be followed. I suspect that the result is that the vast majority of users will pass “John Doe” instead of “Doe^John” and end up with incorrectly formatted attributes.

I consider these formatting details to be far lower level than users of highdicom should have to understand in order to create objects with correctly formatted PN attributes.

I am happy to work on a solution. Here are a few options that come to mind:

(My preferred solution): Create a PersonName object (either in the highdicom.content module or a new highdicom.vrmodule perhaps) with a constructor that takes the five parts of the name (family name, given name, middle name, name prefix, name suffix), any of which can be None, and has a method that returns the correctly formatted string. Then change the API of the various parts of the code expecting person names as string to instead expect PersonName objects.
Continue to expect strings but check that the format is correct (difficult because in theory each component can contain a space and components can be missing)
Try to fix this at the pydicom level.

@hackermd thoughts?

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 21 (1 by maintainers)

Most upvoted comments

My PR to add the functionality to construct PersonNames has now been merged into pydicom master branch: https://github.com/pydicom/pydicom/pull/1331 and will be in the next release of pydicom (looks like this will be 2.2.0). Leaving this issue open to figure out the best way to integrate this into highdicom

CPBridge on Apr 7, 2021

Damn @CPBridge! You are a DICOM pro!🥇

Thanks for your clarification.

On the highdicom side I think it might be reasonable to raise a warning if a value with no ^ characters is passed with a message that the PersonName is likely not what the user intended, but this may be too high level for pydicom

I agree. I think we can/should be opinionated.

hackermd on Mar 31, 2021

I like the approach, but would rely on pydicom.valuerep.PersonName instead. Would that work?

Highdicom callables could then have name: Union[str, pydicom.valuerep.PersonName] and we internally store the attributes using pydicom.valuerep.PersonName.

@hackermd the problem with this approach is that checks enforcing the validity of Person Name attributes is much harder (and probably impossible) than simply forcing people to use a constructor that makes them individually specify each component. E.g. it’s much easier to make sure someone doesn’t enter “John Doe” when they mean “Doe^Joe” by making them pass ‘John’ and ‘Doe’ separately than it is to try and work out after the fact that when they entered “John Doe” they actually meant “Doe^John”. If we assume that it is impossible to implement checks at the pydicom level (even with the enfore_valid_values flag) and then if we do as you suggest and let people pass pydicom.valuerep.PersonName objects, then I imagine what will happen is that everyone will just use the main __init__ method for PersonName and do this:

dcm = Segmentation(
    ...
    content_creator_name=pydicom.valuerep.PersonName("Jonathan Horatio Doe-Smyth, 11th Earl of Dribbling"),
    ...
)

which is a perfectly valid way to create a pydicom.valuerep.PersonName, and we have essentially no way to check this, we’re back at square one.

I would still be tempted to have a highdicom subclass that users of highdicom are required to use when constructing objects that forces users to go through a constructor with components, but then under the hood pass off the construction of the name string to as-yet non-existent functionality in pydicom.

CPBridge on Mar 30, 2021

@hackermd I think when a user import highdicom, we should probably set the pydicom.config.enforce_valid_values to True to make use of the checks that do exist in pydicom. What do you think? Maybe this should go into highdicom.__init__?

CPBridge on Mar 30, 2021

For reference: https://support.dcmtk.org/docs/classDcmPersonName.html

pieper on Mar 28, 2021