Trying to understand GetPrivateTag function of DicomDataset

[aerik]

So I don't think I can say this is a bug, I just may be misunderstanding the code. I read the standard https://dicom.nema.org/medical/dicom/current/output/chtml/part05/sect_7.8.html

and this section in particular (7.8.1.b) :

Private Creator Data Element (gggg,0010) is required in order to identify Data Elements (gggg,1000-10FF) if present, Private Creator Data Element (gggg,0011) is required in order to identify Data Elements (gggg,1100-11FF) if present, through Private Creator Data Element (gggg,00FF), which identifies Data Elements (gggg,FF00-FFFF) if present.

So that says you need a creator element for each chunk of data elements of up to FF size, and that the creator element has to match the first two hex components of all it's corresponding data elements... right?

Okay, so then I go look at the code. The method "GetPrivateTag" is all over DicomDataset

/// <summary>
/// Converts a dictionary tag to a valid private tag.
/// </summary>
/// <param name="tag">Dictionary DICOM tag</param>
/// <param name="createTag">Whether the PrivateCreator tag should be created if needed.</param>
/// <returns>Private DICOM tag, or null if all groups are already used or createTag is false and the
/// PrivateCreator is not already in the dataset. </returns>
internal DicomTag GetPrivateTag(DicomTag tag, bool createTag)
  {
      // not a private tag
      if (!tag.IsPrivate) return tag;
      // group length
      if (tag.Element == 0x0000) return tag;
      // private creator?
      if (tag.PrivateCreator == null) return tag;
      // already a valid private tag
      if (tag.Element > 0xff) return tag;
      ushort group = 0x0010;
      for (; group <= 0x00ff; group++)
      {
          var creator = new DicomTag(tag.Group, group);
          if (!Contains(creator))
          {
              if (!createTag) continue;
              Add(new DicomLongString(creator, tag.PrivateCreator.Creator));
              return new DicomTag(tag.Group, (ushort)((group << 8) + (tag.Element & 0xff)), tag.PrivateCreator);
          }
          var value = TryGetSingleValue(creator, out string tmpValue) ? tmpValue : string.Empty;
          if (tag.PrivateCreator.Creator == value) return new DicomTag(tag.Group, (ushort)((group << 8) + (tag.Element & 0xff)), tag.PrivateCreator);
      }
      return null;
  }

But try as I might, I can't understand the proper functioning of this method. It looks like if I call "DicomDataset.Add(somePrivateTag)" it will potentially change the element that I've passed as a parameter.... right?

Question number two (or three): I cannot find out how I should add a creator tag to the dataset explicitly... it looks like the only way to add a creator tag is via the GetPrivateTag method, and then only inside the loop that is incrementing the group number at the line that says

Add(new DicomLongString(creator, tag.PrivateCreator.Creator));

Is that correct? (that is the only way to add a creator tag?)

The reason I'm asking is that I'm added some private tags in code and this is sort of giving me fits...

[mrbean-bremen]

It looks like if I call "DicomDataset.Add(somePrivateTag)" it will potentially change the element that I've passed as a parameter.... right?

Not really. Admittedly, the name GetPrivateTag may be a bit misleading with the createTag parameter set to true (which is the default, which may not be expected), because in this case it also creates the private creator if it does not exist. But it does not change the private tag as is, because a private tag is defined by the private creator, the group and the tag offset (which is the lower byte of the tag value). The higher byte of the tag (the part what maybe changed here) is not part of the tag definition itself, it just shows which private creator block it belongs to (e.g. this is meta information about the location in the dataset).

I cannot find out how I should add a creator tag to the dataset explicitly

You shouldn't need to. A private creator without a private tag does not make sense, adding a private tag (using DicomDataset.Add) will create it if it does not exist yet.

[aerik]

@mrbean-bremen Thanks for that. After reading your comment, then going through the bit of code that parses the XML into the DicomDictionary(ies), I think I have a bit of a better understanding of what's going on. Within the code there seems to be a convention the private tags are referenced using only the last byte of the element since the first byte is dynamically assigned by GetPrivateTag and in re-reading the relevant section of the standard, I can see why the first byte of the element may be dynamically assigned,.

But then I wonder about the line that allows a private tag with the first byte of the element already set (greater than 0xff) to be passed through without any other checks? Specifically it does not ensure that a creator tag is written to the dataset - and there is no mechanism to add that creator tag manually...

      // already a valid private tag
      if (tag.Element > 0xff) return tag;//this allows a valid private tag to be created without ensuring that the creator tag is added to the dataset

I got into all this because I am programmatically adding private tags rather than adding them via DicomDictionarly.Load, specifically I am using the DicomTag constructors that include a private creator. This let to some unexpected behaviors and all these questions. Is there any use for non-internal calls to these constructors? It seems to me that calling them from code outside of some internal functions is bound to lead to unexpected behavior...

  public DicomTag(ushort group, ushort element, string privateCreator)
      : this(group, element, DicomDictionary.Default.GetPrivateCreator(privateCreator))
  {
  }

  public DicomTag(ushort group, ushort element, DicomPrivateCreator privateCreator)
  {
      Group = group;
      Element = element;
      PrivateCreator = privateCreator;
  }

[mrbean-bremen]

This let to some unexpected behaviors

Can you elaborate what you mean by unexpected behavior?

[aerik]

Ah, I see the convention - the creator tag that you add does not have PrivateCreator set, see this line in GetPrivateTag:

var creator = new DicomTag(tag.Group, group);//no private creator assigned to the creator tag
if (!Contains(creator))
{
    if (!createTag) continue;

    Add(new DicomLongString(creator, tag.PrivateCreator.Creator));//writes the creator tag to the dataset

... and the private creator tag not having the PrivateCreator set makes the comment on this line make sense

// private creator?
if (tag.PrivateCreator == null) return tag;

So that would be how to manually add the creator tag... but I am thinking about doing something like this to ensure the creator tag is written in the case that the tag is "already a valid private tag"

 if (tag.Element > 0xff)
  {
      //check if creator tag exists
      ushort creatorPrefix = (ushort)(tag.Element >> 8);
      DicomTag creatorTag = new DicomTag(tag.Group, creatorPrefix);//don't set PrivateCreator here?
      if (!Contains(creatorTag))
      {
          Add(new DicomLongString(creatorTag, tag.PrivateCreator.Creator));//writes the creator tag to the dataset
      }
      else
      {
          //what if there is a mismatch?
          string creatorString = TryGetSingleValue(creatorTag, out string tmpValue) ? tmpValue : string.Empty;
          if(creatorString != tag.PrivateCreator.Creator)
          {
              //uh-oh... ignore it?
          }
      }
  }

Not sure what to do if there there is a conflict, probably nothing. The existing GetPrivateTag code seems to ignore conflicts / edge cases...

[mrbean-bremen]

but I am thinking about doing something like this to ensure the creator tag is written in the case that the tag is "already a valid private tag"

A private tag without a private creator is not valid, e.g. that would be invalid DICOM. Without a private creator, you cannot know where it belongs to, as the same tag has different meanings for different private creators.

If you write the private tag yourself, fo-dicom (or any other framework) will write the private creator for you, if it does not exist. If you encounter a private tag without a private creator, it would be invalid and should be discarded.

[aerik]

A private tag without a private creator is not valid, e.g. that would be invalid DICOM. Without a private creator, you cannot know where it belongs to, as the same tag has different meanings for different private creators.

If you write the private tag yourself, fo-dicom (or any other framework) will write the private creator for you, if it does not exist. If you encounter a private tag without a private creator, it would be invalid and should be discarded.

Ah, see, that is exactly where you get "unexpected behavior".

Take this example where you register a private creator, create a valid private tag, and then add it to the dataset

  DicomPrivateCreator myCreator = DicomDictionary.Default.GetPrivateCreator("ABC PACS Product");
  DicomTag myPrivateTag = new DicomTag(0x0123, 0x1012, myCreator);
  DicomDataset myDataset = new DicomDataset();
  myDataset.Add(new DicomShortString(myPrivateTag, "TEST"));

The private tag is written to the dataset but without any creator tag. Perhaps I am missing a non-obvious step?

[mrbean-bremen]

I see. That may indeed be unexpected. The crucial point here is that you are creating a private DICOM tag which already has a position in the group (e.g. belongs to a private creator block), because it has a non-zero high byte. In this case fo-dicom assumes that you know what you are doing, and that the private creator already exists. If you create your private tag instead by defining only the tag offset (e.g. no upper byte):

DicomTag myPrivateTag = new DicomTag(0x0123, 0x12, myCreator);

it should work as expected (e.g. create the private creator).

[aerik]

Okay, well I'm glad there's nothing I'm missing.

fo-dicom assumes that you know what you are doing, and that the private creator already exists.

Just to be clear, that means the private creator tag already exists...

If you create your private tag instead by defining only the tag offset ... it should work as expected (e.g. create the private creator).

Thank you, yes, I see that, but I find it terrifcally un-intuitive to create a private tag and have to remember to exclude the upper byte. It seems to me that if that is the convention then the public constructor for a DicomTag with a private creator should be

public DicomTag(ushort group, byte elementOffset, DicomPrivateCreator privateCreator)

However, having said that, I like my earlier suggestion much better: When attempting to write a private tag with the upper byte set, check to see if there is a matching private creator tag and see if it needs to be written - that makes it behave sort of like the existing code but still assumes "you know what you are doing".

Thanks for all the discussion.

[mrbean-bremen]

Just to be clear, that means the private creator tag already exists...

Yes, of course.

I find it terrifcally un-intuitive to create a private tag and have to remember to exclude the upper byte. It seems to me that if that is the convention then the public constructor for a DicomTag with a private creator should be ...

Yes, I agree. As the upper byte is not part of the private tag definition, the behavior is unclear if it is provided.

When attempting to write a private tag with the upper byte set, check to see if there is a matching private creator tag and see if it needs to be written

Actually, if the upper byte is set, and the private creator at that position does not match the provided private creator, I would consider that an error.

[aerik]

Sounds good, thanks again for the discussion.