Use innolitics DICOM Standard JSON dump to generate tags. #316

lnogueir · 2024-05-27T15:05:47Z

This PR is resolving #147.

It updates the generate_tag_definitions.py script to read the DICOM standard JSON dump from the Innolitics repo. They provide a few extra fields, so we introduce them in Tag definition, namely, Keyword and Retired. Keyword represents what Name used 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.

element.go

lnogueir · 2024-05-27T15:41:05Z

pkg/tag/tag.go

@@ -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


Should we make this a []string now that we consider all possible VRs accepted?

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" ... )

suyashkumar

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).

suyashkumar · 2024-05-28T02:50:49Z

pkg/tag/tag.go

@@ -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


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" ... )

write.go

pkg/tag/generate_tag_definitions.py

read.go

pkg/tag/generate_tag_definitions.py

element.go

pkg/tag/tag_test.go

lnogueir · 2024-05-29T01:43:59Z

About the refactor to enum VRs, I think that deservers its own PR.

…packages

…instead of current

…readable

suyashkumar

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/generate_tag_definitions.py

pkg/tag/tag_test.go

suyashkumar · 2024-05-29T04:31:40Z

pkg/tag/tag_test.go

+		t.Run(test.expectedKeyword, func(t *testing.T) {
+			elem, err := Find(test.tag)
+			if err != nil {
+				t.Fatalf("Find returned unecpected error: %v", err)


spelling

Suggested change

t.Fatalf("Find returned unecpected error: %v", err)

t.Fatalf("Find returned unexpected error: %v", err)

suyashkumar · 2024-05-29T04:43:00Z

read.go

+				// 2. https://dicom.nema.org/medical/dicom/2024a/output/html/part05.html#sect_8.2
+				return "OW", nil
+			default:
+				return entry.VRs[0], nil


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).

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.

pkg/tag/generate_tag_definitions.py

suyashkumar

Great work! Thanks for the contribution!

suyashkumar · 2024-05-29T16:53:22Z

read.go

+				// 2. https://dicom.nema.org/medical/dicom/2024a/output/html/part05.html#sect_8.2
+				return "OW", nil
+			default:
+				return entry.VRs[0], nil


lnogueir commented May 27, 2024

View reviewed changes

element.go Show resolved Hide resolved

lnogueir commented May 27, 2024

View reviewed changes

lnogueir force-pushed the issue-147 branch from c3bb8f9 to 1a006d1 Compare May 27, 2024 18:07

suyashkumar reviewed May 28, 2024

View reviewed changes

pkg/tag/tag_test.go Outdated Show resolved Hide resolved

lnogueir added 2 commits May 28, 2024 14:53

Use innolitics DICOM Standard JSON dump to generate tags.

dbdd018

Edit test to ensure we can now write Pixel Data with OB VR

efbf3d1

lnogueir force-pushed the issue-147 branch from 1a006d1 to a7f6097 Compare May 29, 2024 01:31

lnogueir added 5 commits May 29, 2024 01:45

Use urllib instead of requests so user doesn't need to install extra …

03c1adf

…packages

Make link references of the DICOM standard point to specific version …

86248f9

…instead of current

Use f-string when generating tag definition file to make script more …

50f4ea1

…readable

Make tag info vr field an array of strings and improve tag find tests

635a044

Deprecate FindByName in favor of FindByKeyword

13a69fa

lnogueir force-pushed the issue-147 branch from a7f6097 to 13a69fa Compare May 29, 2024 01:46

lnogueir requested a review from suyashkumar May 29, 2024 01:48

suyashkumar approved these changes May 29, 2024

View reviewed changes

suyashkumar self-assigned this May 29, 2024

suyashkumar added the enhancement New feature or request label May 29, 2024

lnogueir added 2 commits May 29, 2024 14:36

rename test variable from expected->want, fix typo

221bd7f

add innolitics license and make generate script more readable

17e30f5

suyashkumar approved these changes May 29, 2024

View reviewed changes

suyashkumar merged commit b290e17 into suyashkumar:main May 29, 2024
2 checks passed

This was referenced May 29, 2024

Rewrite Generated Tag Package #147

Closed

dicom.Write: The Pixel data only supports OW type #299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use innolitics DICOM Standard JSON dump to generate tags. #316

Use innolitics DICOM Standard JSON dump to generate tags. #316

lnogueir commented May 27, 2024 •

edited

Loading

lnogueir May 27, 2024

suyashkumar May 28, 2024

suyashkumar left a comment •

edited

Loading

suyashkumar May 28, 2024

lnogueir commented May 29, 2024

suyashkumar left a comment

suyashkumar May 29, 2024

suyashkumar May 29, 2024

lnogueir May 29, 2024 •

edited

Loading

suyashkumar May 29, 2024

suyashkumar left a comment •

edited

Loading

suyashkumar May 29, 2024

	t.Fatalf("Find returned unecpected error: %v", err)
	t.Fatalf("Find returned unexpected error: %v", err)

Use innolitics DICOM Standard JSON dump to generate tags. #316

Use innolitics DICOM Standard JSON dump to generate tags. #316

Conversation

lnogueir commented May 27, 2024 • edited Loading

lnogueir May 27, 2024

Choose a reason for hiding this comment

suyashkumar May 28, 2024

Choose a reason for hiding this comment

suyashkumar left a comment • edited Loading

Choose a reason for hiding this comment

suyashkumar May 28, 2024

Choose a reason for hiding this comment

lnogueir commented May 29, 2024

suyashkumar left a comment

Choose a reason for hiding this comment

suyashkumar May 29, 2024

Choose a reason for hiding this comment

suyashkumar May 29, 2024

Choose a reason for hiding this comment

lnogueir May 29, 2024 • edited Loading

Choose a reason for hiding this comment

suyashkumar May 29, 2024

Choose a reason for hiding this comment

suyashkumar left a comment • edited Loading

Choose a reason for hiding this comment

suyashkumar May 29, 2024

Choose a reason for hiding this comment

lnogueir commented May 27, 2024 •

edited

Loading

suyashkumar left a comment •

edited

Loading

lnogueir May 29, 2024 •

edited

Loading

suyashkumar left a comment •

edited

Loading