Use innolitics DICOM Standard JSON dump to generate tags. #316
Conversation
pkg/tag/tag.go
Outdated
| @@ -84,15 +84,20 @@ func (t Tags) Contains(item *Tag) bool { | |||
| type Info struct { | |||
| Tag Tag | |||
| // Data encoding "UL", "CS", etc. | |||
| // For tags with multiple allowed VRs, they are separated as "VR1 or VR2 or ... or VRn". | |||
| VR string | |||
There was a problem hiding this comment.
Should we make this a []string now that we consider all possible VRs accepted?
There was a problem hiding this comment.
Yes, lets go ahead and feel free to make necessary API changes that suit the new layout. After we get the next batch of larger changes in I'll do at least a minor release version bump with a clear indication there are API-breaking changes (not that I expect many folks rely on this directly, so it's a bit safer to make a change here).
If we can enumerate all possible VRs easily, we may want to make an aliased string type in Go to represent this VR, and deprecate VRKind.
type VR string
var (
OtherWordString VR = "OW"
...
)
There was a problem hiding this comment.
Overall LGTM, thank you @lnogueir for the contribution and the cleanups as well (certainly there will be many more legacy things to clean up :)). Just had a couple minor comments.
If you rebase on the latest main branch, I think the current test issues should resolve (EDIT: it seems like one of the tag tests may also need updating as well).
pkg/tag/tag.go
Outdated
| @@ -84,15 +84,20 @@ func (t Tags) Contains(item *Tag) bool { | |||
| type Info struct { | |||
| Tag Tag | |||
| // Data encoding "UL", "CS", etc. | |||
| // For tags with multiple allowed VRs, they are separated as "VR1 or VR2 or ... or VRn". | |||
| VR string | |||
There was a problem hiding this comment.
Yes, lets go ahead and feel free to make necessary API changes that suit the new layout. After we get the next batch of larger changes in I'll do at least a minor release version bump with a clear indication there are API-breaking changes (not that I expect many folks rely on this directly, so it's a bit safer to make a change here).
If we can enumerate all possible VRs easily, we may want to make an aliased string type in Go to represent this VR, and deprecate VRKind.
type VR string
var (
OtherWordString VR = "OW"
...
)
|
About the refactor to enum VRs, I think that deservers its own PR. |
…instead of current
There was a problem hiding this comment.
Thanks @lnogueir this looks good to me. Couple more comments and then we should hopefully be ready to merge. Thanks for the contribution!
pkg/tag/tag_test.go
Outdated
| t.Run(test.expectedKeyword, func(t *testing.T) { | ||
| elem, err := Find(test.tag) | ||
| if err != nil { | ||
| t.Fatalf("Find returned unecpected error: %v", err) |
There was a problem hiding this comment.
spelling
| t.Fatalf("Find returned unecpected error: %v", err) | |
| t.Fatalf("Find returned unexpected error: %v", err) |
| // 2. https://dicom.nema.org/medical/dicom/2024a/output/html/part05.html#sect_8.2 | ||
| return "OW", nil | ||
| default: | ||
| return entry.VRs[0], nil |
There was a problem hiding this comment.
out of curiosity, in the case of multiple VRs for a tag in an implicit transfer syntax, does the spec prescribe what to do (e.g. is the first significant here?). This seems fine for now since I'm not sure how else we'd be able to infer the VR more intelligently, even if we tried to inspect the value (may work in some cases).
There was a problem hiding this comment.
IIRC, there were about 40-50 with multiple allowed VRs. Maybe lets create an issue to dig deeper for when these tags show up with implicit. We should be able to find something in the specs. We are covering the main case with Pixel/Overlay Data tags, so not very urgent IMO.
| // 2. https://dicom.nema.org/medical/dicom/2024a/output/html/part05.html#sect_8.2 | ||
| return "OW", nil | ||
| default: | ||
| return entry.VRs[0], nil |
This PR is resolving #147.
It updates the
generate_tag_definitions.pyscript to read the DICOM standard JSON dump from the Innolitics repo. They provide a few extra fields, so we introduce them inTagdefinition, namely,KeywordandRetired.Keywordrepresents whatNameused to represent, see comments on the field definitions for details.Innolitics also captures all possible VRs for tags that allow multiple VR values, so I updated the writer verification code to consider that. That ended up resolving #299 as well.
This is a breaking change because some tag variables have been deleted or renamed. In particular, the command tags (group 0000) have been all deleted since Innolitics doesn't provide them. I thought that was okay because these are only related to DIMSE and should never be present in a DICOM file. I can put them back somehow if you think we should keep them, but maybe we could sort that out if we ever work on #181. But mostly, tags were added or unchanged.
I ended up writing this from scratch and not branching from #169 because I thought that was a little bit too complicated for this. But we could reconsider that design if there are requests for supporting private data dictionaries.
Misc: fixes some warnings (i.e. removed deprecated ioutil, etc.), added a helper Uint32 method.