From 6292dedbfe5a2297c8c87cfafc1c419e8b5eb3c2 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 29 Mar 2021 14:57:46 -0700 Subject: [PATCH 1/4] added IsEmpty method to Value implementations --- element.go | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 4 deletions(-) diff --git a/element.go b/element.go index 8678b4d7..60d2a5d0 100644 --- a/element.go +++ b/element.go @@ -70,6 +70,21 @@ type Value interface { // ValueType returns the underlying ValueType of this Value. This can be used to unpack the underlying data in this // Value. ValueType() ValueType + // Is Empty returns true if the value is an 'empty' value. The following values are + // considered 'empty': + // + // A "" string value. + // + // A []byte value of 0 length + // + // A SequenceItemValue with no elements, or where all sub-elements are empty. + // + // A []SequenceItemValue with no items or where all items are empty. + // + // A PixelDataInfo value with 0 frames. + // + // All other values, including numeric 0 values, are considered 'non-empty' + IsEmpty() bool // GetValue returns the underlying value that this Value holds. What type is returned here can be determined exactly // from the ValueType() of this Value (see the ValueType godoc). GetValue() interface{} // TODO: rename to Get to read cleaner @@ -181,6 +196,7 @@ type bytesValue struct { func (b *bytesValue) isElementValue() {} func (b *bytesValue) ValueType() ValueType { return Bytes } +func (b *bytesValue) IsEmpty() bool { return len(b.value) == 0 } func (b *bytesValue) GetValue() interface{} { return b.value } func (b *bytesValue) String() string { return fmt.Sprintf("%v", b.value) @@ -194,8 +210,19 @@ type stringsValue struct { value []string } -func (s *stringsValue) isElementValue() {} -func (s *stringsValue) ValueType() ValueType { return Strings } +func (s *stringsValue) isElementValue() {} +func (s *stringsValue) ValueType() ValueType { return Strings } +func (s *stringsValue) IsEmpty() bool { + // If any of our string values are not an empty string, this value is not an + // empty value. + for _, value := range s.value { + if value != "" { + return false + } + } + + return true +} func (s *stringsValue) GetValue() interface{} { return s.value } func (s *stringsValue) String() string { return fmt.Sprintf("%v", s.value) @@ -211,6 +238,7 @@ type intsValue struct { func (s *intsValue) isElementValue() {} func (s *intsValue) ValueType() ValueType { return Ints } +func (s *intsValue) IsEmpty() bool { return false } func (s *intsValue) GetValue() interface{} { return s.value } func (s *intsValue) String() string { return fmt.Sprintf("%v", s.value) @@ -226,6 +254,7 @@ type floatsValue struct { func (s *floatsValue) isElementValue() {} func (s *floatsValue) ValueType() ValueType { return Floats } +func (s *floatsValue) IsEmpty() bool { return false } func (s *floatsValue) GetValue() interface{} { return s.value } func (s *floatsValue) String() string { return fmt.Sprintf("%v", s.value) @@ -247,6 +276,21 @@ func (s *SequenceItemValue) isElementValue() {} // to unpack the underlying data in this Value. func (s *SequenceItemValue) ValueType() ValueType { return SequenceItem } +func (s *SequenceItemValue) IsEmpty() bool { + if s.elements == nil || len(s.elements) == 0 { + return true + } + + // If any of our sub-elements are not empty, this SequenceItemValue is not empty. + for _, element := range s.elements { + if !element.Value.IsEmpty() { + return false + } + } + + return true +} + // GetValue returns the underlying value that this Value holds. What type is // returned here can be determined exactly from the ValueType() of this Value // (see the ValueType godoc). @@ -268,8 +312,22 @@ type sequencesValue struct { value []*SequenceItemValue } -func (s *sequencesValue) isElementValue() {} -func (s *sequencesValue) ValueType() ValueType { return Sequences } +func (s *sequencesValue) isElementValue() {} +func (s *sequencesValue) ValueType() ValueType { return Sequences } +func (s *sequencesValue) IsEmpty() bool { + if s.value == nil || len(s.value) == 0 { + return true + } + + // If any of our sequence items are not empty, this SequenceItemValue is not empty. + for _, thisItem := range s.value { + if !thisItem.IsEmpty() { + return false + } + } + + return true +} func (s *sequencesValue) GetValue() interface{} { return s.value } func (s *sequencesValue) String() string { // TODO: consider adding more sophisticated formatting @@ -293,6 +351,7 @@ type pixelDataValue struct { func (e *pixelDataValue) isElementValue() {} func (e *pixelDataValue) ValueType() ValueType { return PixelData } +func (e *pixelDataValue) IsEmpty() bool { return len(e.PixelDataInfo.Frames) == 0 } func (e *pixelDataValue) GetValue() interface{} { return e.PixelDataInfo } func (e *pixelDataValue) String() string { // TODO: consider adding more sophisticated formatting From 1533c1055aacaeafd84c7087887a5383d6e439ef Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 29 Mar 2021 19:41:11 -0700 Subject: [PATCH 2/4] tweaked IsEmpty to return true on 0 int and float values --- element.go | 45 ++++++++--- element_test.go | 208 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 242 insertions(+), 11 deletions(-) diff --git a/element.go b/element.go index 60d2a5d0..a050fbd7 100644 --- a/element.go +++ b/element.go @@ -70,12 +70,16 @@ type Value interface { // ValueType returns the underlying ValueType of this Value. This can be used to unpack the underlying data in this // Value. ValueType() ValueType - // Is Empty returns true if the value is an 'empty' value. The following values are + // IsEmpty returns true if the value is an 'empty' value. The following values are // considered 'empty': // - // A "" string value. + // A []string value where all sub-values are "". // - // A []byte value of 0 length + // A []int value where all sub-values are 0. + // + // A []float64 value where all sub-values are 0.0. + // + // A []byte value of 0 length. // // A SequenceItemValue with no elements, or where all sub-elements are empty. // @@ -83,7 +87,8 @@ type Value interface { // // A PixelDataInfo value with 0 frames. // - // All other values, including numeric 0 values, are considered 'non-empty' + // Make special note of the []int and []float behavior, as for some tags, 0 is a + // meaningful value. IsEmpty() bool // GetValue returns the underlying value that this Value holds. What type is returned here can be determined exactly // from the ValueType() of this Value (see the ValueType godoc). @@ -196,7 +201,7 @@ type bytesValue struct { func (b *bytesValue) isElementValue() {} func (b *bytesValue) ValueType() ValueType { return Bytes } -func (b *bytesValue) IsEmpty() bool { return len(b.value) == 0 } +func (b *bytesValue) IsEmpty() bool { return b.value == nil || len(b.value) == 0 } func (b *bytesValue) GetValue() interface{} { return b.value } func (b *bytesValue) String() string { return fmt.Sprintf("%v", b.value) @@ -236,9 +241,18 @@ type intsValue struct { value []int } -func (s *intsValue) isElementValue() {} -func (s *intsValue) ValueType() ValueType { return Ints } -func (s *intsValue) IsEmpty() bool { return false } +func (s *intsValue) isElementValue() {} +func (s *intsValue) ValueType() ValueType { return Ints } +func (s *intsValue) IsEmpty() bool { + // If any of our string values are not 0, this value is not an empty value. + for _, value := range s.value { + if value != 0 { + return false + } + } + + return true +} func (s *intsValue) GetValue() interface{} { return s.value } func (s *intsValue) String() string { return fmt.Sprintf("%v", s.value) @@ -252,9 +266,18 @@ type floatsValue struct { value []float64 } -func (s *floatsValue) isElementValue() {} -func (s *floatsValue) ValueType() ValueType { return Floats } -func (s *floatsValue) IsEmpty() bool { return false } +func (s *floatsValue) isElementValue() {} +func (s *floatsValue) ValueType() ValueType { return Floats } +func (s *floatsValue) IsEmpty() bool { + // If any of our string values are not 0, this value is not an empty value. + for _, value := range s.value { + if value != 0 { + return false + } + } + + return true +} func (s *floatsValue) GetValue() interface{} { return s.value } func (s *floatsValue) String() string { return fmt.Sprintf("%v", s.value) diff --git a/element_test.go b/element_test.go index a5d88a41..f57d02fc 100644 --- a/element_test.go +++ b/element_test.go @@ -2,6 +2,7 @@ package dicom import ( "encoding/json" + "github.com/suyashkumar/dicom/pkg/frame" "testing" "github.com/google/go-cmp/cmp" @@ -137,6 +138,213 @@ func TestNewValue(t *testing.T) { } } +func TestValue_IsEmpty(t *testing.T) { + cases := []struct { + name string + value Value + expectedIsEmpty bool + }{ + // STRINGS VALUE + { + name: "strings_none_empty", + value: &stringsValue{value: []string{"a", "b"}}, + expectedIsEmpty: false, + }, + { + name: "strings_one_empty", + value: &stringsValue{value: []string{"a", ""}}, + expectedIsEmpty: false, + }, + { + name: "strings_all_empty", + value: &stringsValue{value: []string{"", ""}}, + expectedIsEmpty: true, + }, + { + name: "strings_single_empty", + value: &stringsValue{value: []string{""}}, + expectedIsEmpty: true, + }, + + // INTS VALUE + { + name: "ints_none_empty", + value: &intsValue{value: []int{1, 2}}, + expectedIsEmpty: false, + }, + { + name: "ints_one_empty", + value: &intsValue{value: []int{0, 2}}, + expectedIsEmpty: false, + }, + { + name: "ints_all_empty", + value: &intsValue{value: []int{0, 0}}, + expectedIsEmpty: true, + }, + { + name: "ints_single_empty", + value: &intsValue{value: []int{0}}, + expectedIsEmpty: true, + }, + + // FLOATS VALUE + { + name: "floats_none_empty", + value: &floatsValue{value: []float64{1, 2}}, + expectedIsEmpty: false, + }, + { + name: "floats_one_empty", + value: &floatsValue{value: []float64{0, 2}}, + expectedIsEmpty: false, + }, + { + name: "floats_all_empty", + value: &floatsValue{value: []float64{0, 0}}, + expectedIsEmpty: true, + }, + { + name: "floats_single_empty", + value: &floatsValue{value: []float64{0}}, + expectedIsEmpty: true, + }, + + // BYTES + { + name: "bytes_not_empty", + value: &bytesValue{value: []byte{0x0, 0x1}}, + expectedIsEmpty: false, + }, + { + name: "bytes_empty", + value: &bytesValue{value: []byte{}}, + expectedIsEmpty: true, + }, + + // PIXEL DATA + { + name: "PixelData_empty", + value: &pixelDataValue{PixelDataInfo{IsEncapsulated: true}}, + expectedIsEmpty: true, + }, + { + name: "PixelData_not_empty", + value: &pixelDataValue{ + PixelDataInfo{ + Frames: []frame.Frame{ + {}, + }, + IsEncapsulated: true, + }, + }, + expectedIsEmpty: false, + }, + + // SEQUENCES + { + name: "sequence_empty", + expectedIsEmpty: true, + value: &sequencesValue{value: []*SequenceItemValue{}}, + }, + { + name: "sequence_empty_sub_elements", + expectedIsEmpty: true, + value: &sequencesValue{value: []*SequenceItemValue{ + { + elements: []*Element{ + { + Tag: tag.PatientName, + ValueRepresentation: tag.VRString, + Value: &stringsValue{ + value: []string{""}, + }, + }, + }, + }, + }}, + }, + { + name: "sequence_multiple_empty_sub_elements", + expectedIsEmpty: true, + value: &sequencesValue{value: []*SequenceItemValue{ + { + elements: []*Element{ + { + Tag: tag.PatientName, + ValueRepresentation: tag.VRString, + Value: &stringsValue{ + value: []string{""}, + }, + }, + { + Tag: tag.SOPInstanceUID, + ValueRepresentation: tag.VRString, + Value: &stringsValue{ + value: []string{""}, + }, + }, + }, + }, + }}, + }, + { + name: "sequence_not_empty", + expectedIsEmpty: false, + value: &sequencesValue{value: []*SequenceItemValue{ + { + elements: []*Element{ + { + Tag: tag.PatientName, + ValueRepresentation: tag.VRString, + Value: &stringsValue{ + value: []string{"Bob"}, + }, + }, + }, + }, + }}, + }, + { + name: "sequence_one_empty_sub_elements", + expectedIsEmpty: false, + value: &sequencesValue{value: []*SequenceItemValue{ + { + elements: []*Element{ + { + Tag: tag.PatientName, + ValueRepresentation: tag.VRString, + Value: &stringsValue{ + value: []string{"bob"}, + }, + }, + { + Tag: tag.SOPInstanceUID, + ValueRepresentation: tag.VRString, + Value: &stringsValue{ + value: []string{""}, + }, + }, + }, + }, + }}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + isEmpty := tc.value.IsEmpty() + if isEmpty != tc.expectedIsEmpty { + t.Errorf( + "Value.IsEmpty() returned %v, expected %v", + isEmpty, + tc.expectedIsEmpty, + ) + } + }) + } +} + func TestNewValue_UnexpectedType(t *testing.T) { data := 10 _, err := NewValue(data) From 49358795eadc7a386ba90abb0e35e586ad3b7a96 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Tue, 30 Mar 2021 10:53:30 -0700 Subject: [PATCH 3/4] changed to IsZero --- element.go | 20 ++++++++++---------- element_test.go | 4 ++-- pkg/personname/groupInfo_test.go | 6 +++--- pkg/personname/info_test.go | 6 +++--- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/element.go b/element.go index a050fbd7..a770a6b9 100644 --- a/element.go +++ b/element.go @@ -89,7 +89,7 @@ type Value interface { // // Make special note of the []int and []float behavior, as for some tags, 0 is a // meaningful value. - IsEmpty() bool + IsZero() bool // GetValue returns the underlying value that this Value holds. What type is returned here can be determined exactly // from the ValueType() of this Value (see the ValueType godoc). GetValue() interface{} // TODO: rename to Get to read cleaner @@ -201,7 +201,7 @@ type bytesValue struct { func (b *bytesValue) isElementValue() {} func (b *bytesValue) ValueType() ValueType { return Bytes } -func (b *bytesValue) IsEmpty() bool { return b.value == nil || len(b.value) == 0 } +func (b *bytesValue) IsZero() bool { return b.value == nil || len(b.value) == 0 } func (b *bytesValue) GetValue() interface{} { return b.value } func (b *bytesValue) String() string { return fmt.Sprintf("%v", b.value) @@ -217,7 +217,7 @@ type stringsValue struct { func (s *stringsValue) isElementValue() {} func (s *stringsValue) ValueType() ValueType { return Strings } -func (s *stringsValue) IsEmpty() bool { +func (s *stringsValue) IsZero() bool { // If any of our string values are not an empty string, this value is not an // empty value. for _, value := range s.value { @@ -243,7 +243,7 @@ type intsValue struct { func (s *intsValue) isElementValue() {} func (s *intsValue) ValueType() ValueType { return Ints } -func (s *intsValue) IsEmpty() bool { +func (s *intsValue) IsZero() bool { // If any of our string values are not 0, this value is not an empty value. for _, value := range s.value { if value != 0 { @@ -268,7 +268,7 @@ type floatsValue struct { func (s *floatsValue) isElementValue() {} func (s *floatsValue) ValueType() ValueType { return Floats } -func (s *floatsValue) IsEmpty() bool { +func (s *floatsValue) IsZero() bool { // If any of our string values are not 0, this value is not an empty value. for _, value := range s.value { if value != 0 { @@ -299,14 +299,14 @@ func (s *SequenceItemValue) isElementValue() {} // to unpack the underlying data in this Value. func (s *SequenceItemValue) ValueType() ValueType { return SequenceItem } -func (s *SequenceItemValue) IsEmpty() bool { +func (s *SequenceItemValue) IsZero() bool { if s.elements == nil || len(s.elements) == 0 { return true } // If any of our sub-elements are not empty, this SequenceItemValue is not empty. for _, element := range s.elements { - if !element.Value.IsEmpty() { + if !element.Value.IsZero() { return false } } @@ -337,14 +337,14 @@ type sequencesValue struct { func (s *sequencesValue) isElementValue() {} func (s *sequencesValue) ValueType() ValueType { return Sequences } -func (s *sequencesValue) IsEmpty() bool { +func (s *sequencesValue) IsZero() bool { if s.value == nil || len(s.value) == 0 { return true } // If any of our sequence items are not empty, this SequenceItemValue is not empty. for _, thisItem := range s.value { - if !thisItem.IsEmpty() { + if !thisItem.IsZero() { return false } } @@ -374,7 +374,7 @@ type pixelDataValue struct { func (e *pixelDataValue) isElementValue() {} func (e *pixelDataValue) ValueType() ValueType { return PixelData } -func (e *pixelDataValue) IsEmpty() bool { return len(e.PixelDataInfo.Frames) == 0 } +func (e *pixelDataValue) IsZero() bool { return len(e.PixelDataInfo.Frames) == 0 } func (e *pixelDataValue) GetValue() interface{} { return e.PixelDataInfo } func (e *pixelDataValue) String() string { // TODO: consider adding more sophisticated formatting diff --git a/element_test.go b/element_test.go index f57d02fc..4980a6e9 100644 --- a/element_test.go +++ b/element_test.go @@ -333,10 +333,10 @@ func TestValue_IsEmpty(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - isEmpty := tc.value.IsEmpty() + isEmpty := tc.value.IsZero() if isEmpty != tc.expectedIsEmpty { t.Errorf( - "Value.IsEmpty() returned %v, expected %v", + "Value.IsZero() returned %v, expected %v", isEmpty, tc.expectedIsEmpty, ) diff --git a/pkg/personname/groupInfo_test.go b/pkg/personname/groupInfo_test.go index c53696ab..ed7388b2 100644 --- a/pkg/personname/groupInfo_test.go +++ b/pkg/personname/groupInfo_test.go @@ -83,7 +83,7 @@ func TestNewPersonNameFromDicom(t *testing.T) { Raw string // The parsed information we expect. Expected GroupInfo - // Whether IsEmpty should return true after parsing Raw. + // Whether IsZero should return true after parsing Raw. IsEmpty bool }{ // Full Name @@ -351,7 +351,7 @@ func TestNewPersonNameFromDicom(t *testing.T) { } }) - // Test .IsEmpty() method. + // Test .IsZero() method. t.Run(tc.Raw+"_IsEmpty", func(t *testing.T) { parsed, err := groupFromValueString(tc.Raw, pnGroupAlphabetic) if err != nil { @@ -360,7 +360,7 @@ func TestNewPersonNameFromDicom(t *testing.T) { if tc.IsEmpty != parsed.IsEmpty() { t.Errorf( - ".IsEmpty() returned %v, extected %v", + ".IsZero() returned %v, extected %v", parsed.IsEmpty(), tc.IsEmpty, ) diff --git a/pkg/personname/info_test.go b/pkg/personname/info_test.go index 4cba2004..50365ce1 100644 --- a/pkg/personname/info_test.go +++ b/pkg/personname/info_test.go @@ -11,7 +11,7 @@ func TestInfo(t *testing.T) { Raw string // The parsed information we expect. Expected Info - // Whether IsEmpty should return true after parsing Raw. + // Whether IsZero should return true after parsing Raw. IsEmpty bool }{ // All groups @@ -371,7 +371,7 @@ func TestInfo(t *testing.T) { ) }) - // Test the .IsEmpty() method. + // Test the .IsZero() method. t.Run(tc.Raw+"_isEmpty", func(t *testing.T) { newInfo, err := Parse(tc.Raw) if err != nil { @@ -381,7 +381,7 @@ func TestInfo(t *testing.T) { if tc.IsEmpty != newInfo.IsEmpty() { t.Errorf( - ".IsEmpty(): got '%v', expected '%v'", + ".IsZero(): got '%v', expected '%v'", tc.IsEmpty, newInfo.IsEmpty(), ) From ecf6fb4f9ebae355716ee9f8274b0b2f655f57e3 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Tue, 30 Mar 2021 10:54:59 -0700 Subject: [PATCH 4/4] reverted personname.Info.IsEmpty comment references --- pkg/personname/groupInfo_test.go | 6 +++--- pkg/personname/info_test.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/personname/groupInfo_test.go b/pkg/personname/groupInfo_test.go index ed7388b2..c53696ab 100644 --- a/pkg/personname/groupInfo_test.go +++ b/pkg/personname/groupInfo_test.go @@ -83,7 +83,7 @@ func TestNewPersonNameFromDicom(t *testing.T) { Raw string // The parsed information we expect. Expected GroupInfo - // Whether IsZero should return true after parsing Raw. + // Whether IsEmpty should return true after parsing Raw. IsEmpty bool }{ // Full Name @@ -351,7 +351,7 @@ func TestNewPersonNameFromDicom(t *testing.T) { } }) - // Test .IsZero() method. + // Test .IsEmpty() method. t.Run(tc.Raw+"_IsEmpty", func(t *testing.T) { parsed, err := groupFromValueString(tc.Raw, pnGroupAlphabetic) if err != nil { @@ -360,7 +360,7 @@ func TestNewPersonNameFromDicom(t *testing.T) { if tc.IsEmpty != parsed.IsEmpty() { t.Errorf( - ".IsZero() returned %v, extected %v", + ".IsEmpty() returned %v, extected %v", parsed.IsEmpty(), tc.IsEmpty, ) diff --git a/pkg/personname/info_test.go b/pkg/personname/info_test.go index 50365ce1..4cba2004 100644 --- a/pkg/personname/info_test.go +++ b/pkg/personname/info_test.go @@ -11,7 +11,7 @@ func TestInfo(t *testing.T) { Raw string // The parsed information we expect. Expected Info - // Whether IsZero should return true after parsing Raw. + // Whether IsEmpty should return true after parsing Raw. IsEmpty bool }{ // All groups @@ -371,7 +371,7 @@ func TestInfo(t *testing.T) { ) }) - // Test the .IsZero() method. + // Test the .IsEmpty() method. t.Run(tc.Raw+"_isEmpty", func(t *testing.T) { newInfo, err := Parse(tc.Raw) if err != nil { @@ -381,7 +381,7 @@ func TestInfo(t *testing.T) { if tc.IsEmpty != newInfo.IsEmpty() { t.Errorf( - ".IsZero(): got '%v', expected '%v'", + ".IsEmpty(): got '%v', expected '%v'", tc.IsEmpty, newInfo.IsEmpty(), )