From 1d5ec8db8cb1eab5511ac673fd8147de696e1194 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Sat, 20 Apr 2024 11:14:54 -0500 Subject: [PATCH 01/11] ENH: Add lookup and putting of private elements --- Cargo.lock | 2 +- object/src/lib.rs | 30 +++++++++++- object/src/mem.rs | 115 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 135 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a1596bbe7..eb20ef10a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1963,7 +1963,7 @@ version = "2.9.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "11f214ce18d8b2cbe84ed3aa6486ed3f5b285cf8d8fbdbce9f3f767a724adc35" dependencies = [ - "base64 0.21.5", + "base64 0.21.7", "flate2", "log", "once_cell", diff --git a/object/src/lib.rs b/object/src/lib.rs index 021a41b1f..566aef7f6 100644 --- a/object/src/lib.rs +++ b/object/src/lib.rs @@ -150,7 +150,7 @@ pub use dicom_dictionary_std::StandardDataDictionary; /// The default implementation of a root DICOM object. pub type DefaultDicomObject = FileDicomObject>; -use dicom_core::header::Header; +use dicom_core::header::{ElementNumber, GroupNumber, Header}; use dicom_encoding::adapters::{PixelDataObject, RawPixelData}; use dicom_encoding::transfer_syntax::TransferSyntaxIndex; use dicom_parser::dataset::{DataSetWriter, IntoTokens}; @@ -287,6 +287,34 @@ pub enum WriteError { WriteUnsupportedTransferSyntax { uid: String, backtrace: Backtrace }, } +#[derive(Debug, Snafu)] +#[non_exhaustive] +pub enum PrivateElementError { + #[snafu(display("Element number must be less than 256, got {}", elem))] + InvalidElement{ + elem: ElementNumber + }, + #[snafu(display("Group number must be odd, found {}", group))] + InvalidGroup{ + group: GroupNumber + }, + #[snafu(display("Private creator {} not found in group {}", creator, group))] + PrivateCreatorNotFound { + creator: String, + group: GroupNumber + }, + #[snafu(display("Private Creator {} found in group {}, but elem {} not found", creator, group, elem))] + ElementNotFound { + creator: String, + group: GroupNumber, + elem: ElementNumber + }, + #[snafu(display("No space available in group {}", group))] + NoSpace{ + group: GroupNumber + } +} + /// An error which may occur when looking up a DICOM object's attributes. #[derive(Debug, Snafu)] #[non_exhaustive] diff --git a/object/src/mem.rs b/object/src/mem.rs index 5e7c88586..229477298 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -41,7 +41,7 @@ use dicom_core::ops::{ }; use itertools::Itertools; use smallvec::SmallVec; -use snafu::{OptionExt, ResultExt}; +use snafu::{ensure, OptionExt, ResultExt}; use std::borrow::Cow; use std::fs::File; use std::io::{BufRead, BufReader, Read}; @@ -54,16 +54,10 @@ use crate::ops::{ }; use crate::{meta::FileMetaTable, FileMetaTableBuilder}; use crate::{ - AccessByNameError, AccessError, AtAccessError, BuildMetaTableSnafu, CreateParserSnafu, - CreatePrinterSnafu, DicomObject, FileDicomObject, MissingElementValueSnafu, - MissingLeafElementSnafu, NoSuchAttributeNameSnafu, NoSuchDataElementAliasSnafu, - NoSuchDataElementTagSnafu, NotASequenceSnafu, OpenFileSnafu, ParseMetaDataSetSnafu, - PrematureEndSnafu, PrepareMetaTableSnafu, PrintDataSetSnafu, ReadError, ReadFileSnafu, - ReadPreambleBytesSnafu, ReadTokenSnafu, ReadUnsupportedTransferSyntaxSnafu, - UnexpectedTokenSnafu, WithMetaError, WriteError, + AccessByNameError, AccessError, AtAccessError, BuildMetaTableSnafu, CreateParserSnafu, CreatePrinterSnafu, DicomObject, FileDicomObject, InvalidElementSnafu, InvalidGroupSnafu, MissingElementValueSnafu, MissingLeafElementSnafu, NoSpaceSnafu, NoSuchAttributeNameSnafu, NoSuchDataElementAliasSnafu, NoSuchDataElementTagSnafu, NotASequenceSnafu, OpenFileSnafu, ParseMetaDataSetSnafu, PrematureEndSnafu, PrepareMetaTableSnafu, PrintDataSetSnafu, PrivateCreatorNotFoundSnafu, PrivateElementError, ReadError, ReadFileSnafu, ReadPreambleBytesSnafu, ReadTokenSnafu, ReadUnsupportedTransferSyntaxSnafu, UnexpectedTokenSnafu, WithMetaError, WriteError }; use dicom_core::dictionary::{DataDictionary, DataDictionaryEntry}; -use dicom_core::header::{HasLength, Header}; +use dicom_core::header::{ElementNumber, GroupNumber, HasLength, Header}; use dicom_core::value::{DataSetSequence, PixelFragmentSequence, Value, ValueType, C}; use dicom_core::{DataElement, Length, PrimitiveValue, Tag, VR}; use dicom_dictionary_std::{tags, StandardDataDictionary}; @@ -666,7 +660,7 @@ where Err(super::AccessError::NoSuchDataElementTag { .. }) => Ok(None), } } - + /// Get a particular DICOM attribute from this object by tag. /// /// If the element does not exist, @@ -704,6 +698,28 @@ where } } + fn find_private_creator(&self, group: GroupNumber, creator: &str) -> Option<&Tag>{ + let range = Tag(group, 0)..Tag(group, 0xFF); + for (tag, elem) in self.entries.range(range) { + // Private Creators are always LO + // https://dicom.nema.org/dicom/2013/output/chtml/part05/sect_7.8.html + if elem.header().vr() == VR::LO && elem.to_str().unwrap_or_default() == creator { + return Some(tag) + } + } + None + } + + pub fn private_element(&self, group: GroupNumber, creator: &str, element: ElementNumber) -> Result<&InMemElement, PrivateElementError> { + ensure!(element < 0xFF, InvalidElementSnafu{elem: element}); + let tag = self.find_private_creator(group, creator) + .ok_or(PrivateCreatorNotFoundSnafu{group, creator: creator.to_string()}.build())?; + + let element_num = (tag.element() << 8) | element; + Ok(self.get(Tag(group, element_num)) + .ok_or(PrivateElementError::ElementNotFound{group, creator: creator.to_string(), elem: element})?) + } + /// Insert a data element to the object, replacing (and returning) any /// previous element of the same attribute. /// This might invalidate all sequence and item lengths if the charset of the @@ -721,6 +737,46 @@ where self.invalidate_if_charset_changed(elt.tag()); self.entries.insert(elt.tag(), elt) } + + pub fn put_private_element( + &mut self, + group: GroupNumber, + creator: &str, + element: ElementNumber, + vr: VR, + value: PrimitiveValue + ) -> Result>, PrivateElementError>{ + ensure!(element < 0xFF, InvalidElementSnafu{elem: element}); + ensure!(group % 2 == 1, InvalidGroupSnafu{group}); + let private_creator = self.find_private_creator(group, creator); + if let Some(tag) = private_creator{ + // Private creator already exists + let tag = Tag(group, tag.element() << 8 | element); + return Ok(self.put_element(DataElement::new(tag, vr, value))) + + } else { + + // Find last reserved block of tags. + let range = Tag(group, 0)..Tag(group, 0xFF); + let last_entry = self.entries.range(range).rev().next(); + let next_available = match last_entry { + Some((tag, _)) => tag.element() + 1, + None => 0x01 + }; + if next_available < 0xFF { + // Put private creator + let tag = Tag(group, next_available); + self.put_str(tag, VR::LO, creator); + + // Put private element + let tag = Tag(group, next_available << 8 | element); + return Ok(self.put_element(DataElement::new(tag, vr, value))) + + } else { + return NoSpaceSnafu{group}.fail() + } + } + } /// Insert a new element with a string value to the object, /// replacing (and returning) any previous element of the same attribute. @@ -3626,4 +3682,43 @@ mod tests { converted_tokens ); } + + #[test] + fn private_elements(){ + let mut ds = InMemDicomObject::from_element_iter(vec![ + DataElement::new( + Tag(0x0009, 0x0010), + VR::LO, + PrimitiveValue::from("CREATOR 1"), + ), + DataElement::new( + Tag(0x0009, 0x0011), + VR::LO, + PrimitiveValue::from("CREATOR 2"), + ), + DataElement::new( + Tag(0x0011, 0x0010), + VR::LO, + PrimitiveValue::from("CREATOR 3"), + ), + ]); + ds.put_private_element( + 0x0009, + "CREATOR 1", + 0x01, + VR::DS, + PrimitiveValue::Str("1.0".to_string()) + ).unwrap(); + ds.put_private_element( + 0x0009, + "CREATOR 4", + 0x02, + VR::DS, + PrimitiveValue::Str("1.0".to_string()) + ).unwrap(); + assert_eq!(ds.private_element(0x0009, "CREATOR 1", 0x01).unwrap().value().to_str().unwrap(), "1.0"); + assert_eq!(ds.private_element(0x0009, "CREATOR 4", 0x02).unwrap().value().to_str().unwrap(), "1.0"); + assert_eq!(ds.private_element(0x0009, "CREATOR 4", 0x02).unwrap().header().tag(), Tag(0x0009, 0x1202)); + + } } From 3d7e3b2a93a514f17008a14106586e6fd10c4bf6 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Mon, 22 Apr 2024 11:37:19 -0500 Subject: [PATCH 02/11] MAIN: Review comments * Format object/src/mem.rs and object/src/lib.rs * Change from u16 to u8, removing one error type --- object/src/lib.rs | 26 ++++------ object/src/mem.rs | 124 +++++++++++++++++++++++++++++----------------- 2 files changed, 89 insertions(+), 61 deletions(-) diff --git a/object/src/lib.rs b/object/src/lib.rs index 566aef7f6..ca37afd99 100644 --- a/object/src/lib.rs +++ b/object/src/lib.rs @@ -290,29 +290,23 @@ pub enum WriteError { #[derive(Debug, Snafu)] #[non_exhaustive] pub enum PrivateElementError { - #[snafu(display("Element number must be less than 256, got {}", elem))] - InvalidElement{ - elem: ElementNumber - }, #[snafu(display("Group number must be odd, found {}", group))] - InvalidGroup{ - group: GroupNumber - }, + InvalidGroup { group: GroupNumber }, #[snafu(display("Private creator {} not found in group {}", creator, group))] - PrivateCreatorNotFound { - creator: String, - group: GroupNumber - }, - #[snafu(display("Private Creator {} found in group {}, but elem {} not found", creator, group, elem))] + PrivateCreatorNotFound { creator: String, group: GroupNumber }, + #[snafu(display( + "Private Creator {} found in group {}, but elem {} not found", + creator, + group, + elem + ))] ElementNotFound { creator: String, group: GroupNumber, - elem: ElementNumber + elem: u8, }, #[snafu(display("No space available in group {}", group))] - NoSpace{ - group: GroupNumber - } + NoSpace { group: GroupNumber }, } /// An error which may occur when looking up a DICOM object's attributes. diff --git a/object/src/mem.rs b/object/src/mem.rs index 229477298..85d1ee192 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -54,7 +54,14 @@ use crate::ops::{ }; use crate::{meta::FileMetaTable, FileMetaTableBuilder}; use crate::{ - AccessByNameError, AccessError, AtAccessError, BuildMetaTableSnafu, CreateParserSnafu, CreatePrinterSnafu, DicomObject, FileDicomObject, InvalidElementSnafu, InvalidGroupSnafu, MissingElementValueSnafu, MissingLeafElementSnafu, NoSpaceSnafu, NoSuchAttributeNameSnafu, NoSuchDataElementAliasSnafu, NoSuchDataElementTagSnafu, NotASequenceSnafu, OpenFileSnafu, ParseMetaDataSetSnafu, PrematureEndSnafu, PrepareMetaTableSnafu, PrintDataSetSnafu, PrivateCreatorNotFoundSnafu, PrivateElementError, ReadError, ReadFileSnafu, ReadPreambleBytesSnafu, ReadTokenSnafu, ReadUnsupportedTransferSyntaxSnafu, UnexpectedTokenSnafu, WithMetaError, WriteError + AccessByNameError, AccessError, AtAccessError, BuildMetaTableSnafu, CreateParserSnafu, + CreatePrinterSnafu, DicomObject, ElementNotFoundSnafu, FileDicomObject, InvalidElementSnafu, + InvalidGroupSnafu, MissingElementValueSnafu, MissingLeafElementSnafu, NoSpaceSnafu, + NoSuchAttributeNameSnafu, NoSuchDataElementAliasSnafu, NoSuchDataElementTagSnafu, + NotASequenceSnafu, OpenFileSnafu, ParseMetaDataSetSnafu, PrematureEndSnafu, + PrepareMetaTableSnafu, PrintDataSetSnafu, PrivateCreatorNotFoundSnafu, PrivateElementError, + ReadError, ReadFileSnafu, ReadPreambleBytesSnafu, ReadTokenSnafu, + ReadUnsupportedTransferSyntaxSnafu, UnexpectedTokenSnafu, WithMetaError, WriteError, }; use dicom_core::dictionary::{DataDictionary, DataDictionaryEntry}; use dicom_core::header::{ElementNumber, GroupNumber, HasLength, Header}; @@ -366,8 +373,7 @@ where // read rest of data according to metadata, feed it to object if let Some(ts) = ts_index.get(&meta.transfer_syntax) { - let mut dataset = - DataSetReader::new_with_ts(file, ts).context(CreateParserSnafu)?; + let mut dataset = DataSetReader::new_with_ts(file, ts).context(CreateParserSnafu)?; Ok(FileDicomObject { meta, @@ -451,8 +457,7 @@ where // read rest of data according to metadata, feed it to object if let Some(ts) = ts_index.get(&meta.transfer_syntax) { - let mut dataset = - DataSetReader::new_with_ts(file, ts).context(CreateParserSnafu)?; + let mut dataset = DataSetReader::new_with_ts(file, ts).context(CreateParserSnafu)?; let obj = InMemDicomObject::build_object( &mut dataset, dict, @@ -660,7 +665,7 @@ where Err(super::AccessError::NoSuchDataElementTag { .. }) => Ok(None), } } - + /// Get a particular DICOM attribute from this object by tag. /// /// If the element does not exist, @@ -698,26 +703,41 @@ where } } - fn find_private_creator(&self, group: GroupNumber, creator: &str) -> Option<&Tag>{ + fn find_private_creator(&self, group: GroupNumber, creator: &str) -> Option<&Tag> { let range = Tag(group, 0)..Tag(group, 0xFF); for (tag, elem) in self.entries.range(range) { // Private Creators are always LO // https://dicom.nema.org/dicom/2013/output/chtml/part05/sect_7.8.html if elem.header().vr() == VR::LO && elem.to_str().unwrap_or_default() == creator { - return Some(tag) + return Some(tag); } } None } - pub fn private_element(&self, group: GroupNumber, creator: &str, element: ElementNumber) -> Result<&InMemElement, PrivateElementError> { - ensure!(element < 0xFF, InvalidElementSnafu{elem: element}); - let tag = self.find_private_creator(group, creator) - .ok_or(PrivateCreatorNotFoundSnafu{group, creator: creator.to_string()}.build())?; - - let element_num = (tag.element() << 8) | element; - Ok(self.get(Tag(group, element_num)) - .ok_or(PrivateElementError::ElementNotFound{group, creator: creator.to_string(), elem: element})?) + pub fn private_element( + &self, + group: GroupNumber, + creator: &str, + element: u8, + ) -> Result<&InMemElement, PrivateElementError> { + let tag = self.find_private_creator(group, creator).ok_or( + PrivateCreatorNotFoundSnafu { + group, + creator: creator.to_string(), + } + .build(), + )?; + + let element_num = (tag.element() << 8) | (element as u16); + self.get(Tag(group, element_num)).ok_or( + ElementNotFoundSnafu { + group, + creator: creator.to_string(), + elem: element, + } + .fail()?, + ) } /// Insert a data element to the object, replacing (and returning) any @@ -737,31 +757,28 @@ where self.invalidate_if_charset_changed(elt.tag()); self.entries.insert(elt.tag(), elt) } - + pub fn put_private_element( &mut self, group: GroupNumber, creator: &str, - element: ElementNumber, + element: u8, vr: VR, - value: PrimitiveValue - ) -> Result>, PrivateElementError>{ - ensure!(element < 0xFF, InvalidElementSnafu{elem: element}); - ensure!(group % 2 == 1, InvalidGroupSnafu{group}); + value: PrimitiveValue, + ) -> Result>, PrivateElementError> { + ensure!(group % 2 == 1, InvalidGroupSnafu { group }); let private_creator = self.find_private_creator(group, creator); - if let Some(tag) = private_creator{ + if let Some(tag) = private_creator { // Private creator already exists - let tag = Tag(group, tag.element() << 8 | element); - return Ok(self.put_element(DataElement::new(tag, vr, value))) - + let tag = Tag(group, tag.element() << 8 | (element as u16)); + return Ok(self.put_element(DataElement::new(tag, vr, value))); } else { - // Find last reserved block of tags. let range = Tag(group, 0)..Tag(group, 0xFF); let last_entry = self.entries.range(range).rev().next(); let next_available = match last_entry { Some((tag, _)) => tag.element() + 1, - None => 0x01 + None => 0x01, }; if next_available < 0xFF { // Put private creator @@ -769,11 +786,10 @@ where self.put_str(tag, VR::LO, creator); // Put private element - let tag = Tag(group, next_available << 8 | element); - return Ok(self.put_element(DataElement::new(tag, vr, value))) - + let tag = Tag(group, next_available << 8 | (element as u16)); + return Ok(self.put_element(DataElement::new(tag, vr, value))); } else { - return NoSpaceSnafu{group}.fail() + return NoSpaceSnafu { group }.fail(); } } } @@ -1982,10 +1998,7 @@ mod tests { use byteordered::Endianness; use dicom_core::chrono::FixedOffset; use dicom_core::value::{DicomDate, DicomDateTime, DicomTime}; - use dicom_core::{ - dicom_value, - header::DataElementHeader, - }; + use dicom_core::{dicom_value, header::DataElementHeader}; use dicom_encoding::{ decode::{basic::BasicDecoder, implicit_le::ImplicitVRLittleEndianDecoder}, encode::{implicit_le::ImplicitVRLittleEndianEncoder, EncoderFor}, @@ -3684,7 +3697,7 @@ mod tests { } #[test] - fn private_elements(){ + fn private_elements() { let mut ds = InMemDicomObject::from_element_iter(vec![ DataElement::new( Tag(0x0009, 0x0010), @@ -3707,18 +3720,39 @@ mod tests { "CREATOR 1", 0x01, VR::DS, - PrimitiveValue::Str("1.0".to_string()) - ).unwrap(); + PrimitiveValue::Str("1.0".to_string()), + ) + .unwrap(); ds.put_private_element( 0x0009, "CREATOR 4", 0x02, VR::DS, - PrimitiveValue::Str("1.0".to_string()) - ).unwrap(); - assert_eq!(ds.private_element(0x0009, "CREATOR 1", 0x01).unwrap().value().to_str().unwrap(), "1.0"); - assert_eq!(ds.private_element(0x0009, "CREATOR 4", 0x02).unwrap().value().to_str().unwrap(), "1.0"); - assert_eq!(ds.private_element(0x0009, "CREATOR 4", 0x02).unwrap().header().tag(), Tag(0x0009, 0x1202)); - + PrimitiveValue::Str("1.0".to_string()), + ) + .unwrap(); + assert_eq!( + ds.private_element(0x0009, "CREATOR 1", 0x01) + .unwrap() + .value() + .to_str() + .unwrap(), + "1.0" + ); + assert_eq!( + ds.private_element(0x0009, "CREATOR 4", 0x02) + .unwrap() + .value() + .to_str() + .unwrap(), + "1.0" + ); + assert_eq!( + ds.private_element(0x0009, "CREATOR 4", 0x02) + .unwrap() + .header() + .tag(), + Tag(0x0009, 0x1202) + ); } } From f39e28b4927ccd96212f257c268c9f06a800ab9b Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Mon, 22 Apr 2024 11:40:04 -0500 Subject: [PATCH 03/11] MAIN: Fix unused and no-longer-existent import --- object/src/mem.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 85d1ee192..5381ea2ea 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -55,16 +55,16 @@ use crate::ops::{ use crate::{meta::FileMetaTable, FileMetaTableBuilder}; use crate::{ AccessByNameError, AccessError, AtAccessError, BuildMetaTableSnafu, CreateParserSnafu, - CreatePrinterSnafu, DicomObject, ElementNotFoundSnafu, FileDicomObject, InvalidElementSnafu, - InvalidGroupSnafu, MissingElementValueSnafu, MissingLeafElementSnafu, NoSpaceSnafu, - NoSuchAttributeNameSnafu, NoSuchDataElementAliasSnafu, NoSuchDataElementTagSnafu, - NotASequenceSnafu, OpenFileSnafu, ParseMetaDataSetSnafu, PrematureEndSnafu, - PrepareMetaTableSnafu, PrintDataSetSnafu, PrivateCreatorNotFoundSnafu, PrivateElementError, - ReadError, ReadFileSnafu, ReadPreambleBytesSnafu, ReadTokenSnafu, - ReadUnsupportedTransferSyntaxSnafu, UnexpectedTokenSnafu, WithMetaError, WriteError, + CreatePrinterSnafu, DicomObject, ElementNotFoundSnafu, FileDicomObject, InvalidGroupSnafu, + MissingElementValueSnafu, MissingLeafElementSnafu, NoSpaceSnafu, NoSuchAttributeNameSnafu, + NoSuchDataElementAliasSnafu, NoSuchDataElementTagSnafu, NotASequenceSnafu, OpenFileSnafu, + ParseMetaDataSetSnafu, PrematureEndSnafu, PrepareMetaTableSnafu, PrintDataSetSnafu, + PrivateCreatorNotFoundSnafu, PrivateElementError, ReadError, ReadFileSnafu, + ReadPreambleBytesSnafu, ReadTokenSnafu, ReadUnsupportedTransferSyntaxSnafu, + UnexpectedTokenSnafu, WithMetaError, WriteError, }; use dicom_core::dictionary::{DataDictionary, DataDictionaryEntry}; -use dicom_core::header::{ElementNumber, GroupNumber, HasLength, Header}; +use dicom_core::header::{GroupNumber, HasLength, Header}; use dicom_core::value::{DataSetSequence, PixelFragmentSequence, Value, ValueType, C}; use dicom_core::{DataElement, Length, PrimitiveValue, Tag, VR}; use dicom_dictionary_std::{tags, StandardDataDictionary}; From 4364fde56fb34225f19ae0bab783306133fc5ba5 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Mon, 22 Apr 2024 12:54:28 -0500 Subject: [PATCH 04/11] ENH: Defer error creation Also fixes tests --- object/src/mem.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 5381ea2ea..0a26cd3be 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -721,23 +721,23 @@ where creator: &str, element: u8, ) -> Result<&InMemElement, PrivateElementError> { - let tag = self.find_private_creator(group, creator).ok_or( + let tag = self.find_private_creator(group, creator).ok_or_else(|| { PrivateCreatorNotFoundSnafu { group, creator: creator.to_string(), } - .build(), - )?; + .build() + })?; let element_num = (tag.element() << 8) | (element as u16); - self.get(Tag(group, element_num)).ok_or( + self.get(Tag(group, element_num)).ok_or_else(|| { ElementNotFoundSnafu { group, creator: creator.to_string(), elem: element, } - .fail()?, - ) + .build() + }) } /// Insert a data element to the object, replacing (and returning) any From 568d82c1e77781f7037859f0068a4422ab5ffe95 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Mon, 22 Apr 2024 12:57:02 -0500 Subject: [PATCH 05/11] MAIN: Remove unused import --- object/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/object/src/lib.rs b/object/src/lib.rs index ca37afd99..cc83e0c5c 100644 --- a/object/src/lib.rs +++ b/object/src/lib.rs @@ -150,7 +150,7 @@ pub use dicom_dictionary_std::StandardDataDictionary; /// The default implementation of a root DICOM object. pub type DefaultDicomObject = FileDicomObject>; -use dicom_core::header::{ElementNumber, GroupNumber, Header}; +use dicom_core::header::{GroupNumber, Header}; use dicom_encoding::adapters::{PixelDataObject, RawPixelData}; use dicom_encoding::transfer_syntax::TransferSyntaxIndex; use dicom_parser::dataset::{DataSetWriter, IntoTokens}; From a02c45f9c3fbb0227d78fdd7acdefbf558c04dd3 Mon Sep 17 00:00:00 2001 From: Nathan Richman <68390038+naterichman@users.noreply.github.com> Date: Tue, 23 Apr 2024 09:55:23 -0500 Subject: [PATCH 06/11] Apply suggestions from code review Co-authored-by: Eduardo Pinho --- object/src/mem.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 0a26cd3be..f63794874 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -707,7 +707,7 @@ where let range = Tag(group, 0)..Tag(group, 0xFF); for (tag, elem) in self.entries.range(range) { // Private Creators are always LO - // https://dicom.nema.org/dicom/2013/output/chtml/part05/sect_7.8.html + // https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html if elem.header().vr() == VR::LO && elem.to_str().unwrap_or_default() == creator { return Some(tag); } @@ -771,11 +771,11 @@ where if let Some(tag) = private_creator { // Private creator already exists let tag = Tag(group, tag.element() << 8 | (element as u16)); - return Ok(self.put_element(DataElement::new(tag, vr, value))); + Ok(self.put_element(DataElement::new(tag, vr, value))) } else { // Find last reserved block of tags. let range = Tag(group, 0)..Tag(group, 0xFF); - let last_entry = self.entries.range(range).rev().next(); + let last_entry = self.entries.range(range).next_back(); let next_available = match last_entry { Some((tag, _)) => tag.element() + 1, None => 0x01, @@ -787,9 +787,9 @@ where // Put private element let tag = Tag(group, next_available << 8 | (element as u16)); - return Ok(self.put_element(DataElement::new(tag, vr, value))); + Ok(self.put_element(DataElement::new(tag, vr, value))) } else { - return NoSpaceSnafu { group }.fail(); + NoSpaceSnafu { group }.fail() } } } From 611ea815a71fc7661f43167188d142c36548bfef Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Tue, 23 Apr 2024 10:37:38 -0500 Subject: [PATCH 07/11] MAIN: Review comments * Add docs for new public functions * Make the PrivateElementError enum represent tags as hex * Add some tests for error representation --- object/src/lib.rs | 8 ++--- object/src/mem.rs | 91 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/object/src/lib.rs b/object/src/lib.rs index cc83e0c5c..9cf3d4219 100644 --- a/object/src/lib.rs +++ b/object/src/lib.rs @@ -290,12 +290,12 @@ pub enum WriteError { #[derive(Debug, Snafu)] #[non_exhaustive] pub enum PrivateElementError { - #[snafu(display("Group number must be odd, found {}", group))] + #[snafu(display("Group number must be odd, found {:#06x}", group))] InvalidGroup { group: GroupNumber }, - #[snafu(display("Private creator {} not found in group {}", creator, group))] + #[snafu(display("Private creator {} not found in group {:#06x}", creator, group))] PrivateCreatorNotFound { creator: String, group: GroupNumber }, #[snafu(display( - "Private Creator {} found in group {}, but elem {} not found", + "Private Creator {} found in group {:#06x}, but elem {:#06x} not found", creator, group, elem @@ -305,7 +305,7 @@ pub enum PrivateElementError { group: GroupNumber, elem: u8, }, - #[snafu(display("No space available in group {}", group))] + #[snafu(display("No space available in group {:#06x}", group))] NoSpace { group: GroupNumber }, } diff --git a/object/src/mem.rs b/object/src/mem.rs index f63794874..9f9d481f6 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -715,6 +715,33 @@ where None } + /// Get a private element from the dataset using the group number, creator and element number. + /// + /// For more info, see the [DICOM standard section on private elements](https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html) + /// + /// ## Example + /// + /// ``` + /// # use dicom_core::{VR, PrimitiveValue, Tag, DataElement}; + /// # use dicom_object::InMemDicomObject; + /// + /// let mut ds = InMemDicomObject::from_element_iter(vec![ + /// DataElement::new( + /// Tag(0x0009, 0x0010), + /// VR::LO, + /// PrimitiveValue::from("CREATOR 1"), + /// ), + /// DataElement::new(Tag(0x0009, 0x01001), VR::DS, PrimitiveValue::from("1.0")), + /// ]); + /// assert_eq!( + /// ds.private_element(0x0009, "CREATOR 1", 0x01) + /// .unwrap() + /// .value() + /// .to_str() + /// .unwrap(), + /// "1.0" + /// ); + /// ``` pub fn private_element( &self, group: GroupNumber, @@ -758,6 +785,45 @@ where self.entries.insert(elt.tag(), elt) } + /// Insert a private element into the dataset, replacing (and returning) any + /// previous element of the same attribute. + /// + /// This function will find the next available private element block in the given + /// group. If the creator already exists, the element will be added to the block + /// already reserved for that creator. If it does not exist, then a new block + /// will be reserved for the creator in the specified group. + /// + /// For more info, see the [DICOM standard section on private elements](https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html) + /// + /// ## Example + /// ``` + /// # use dicom_core::{VR, PrimitiveValue, Tag, DataElement, header::Header}; + /// # use dicom_object::InMemDicomObject; + /// let mut ds = InMemDicomObject::new_empty(); + /// ds.put_private_element( + /// 0x0009, + /// "CREATOR 1", + /// 0x02, + /// VR::DS, + /// PrimitiveValue::Str("1.0".to_string()), + /// ) + /// .unwrap(); + /// assert_eq!( + /// ds.private_element(0x0009, "CREATOR 1", 0x02) + /// .unwrap() + /// .value() + /// .to_str() + /// .unwrap(), + /// "1.0" + /// ); + /// assert_eq!( + /// ds.private_element(0x0009, "CREATOR 1", 0x02) + /// .unwrap() + /// .header() + /// .tag(), + /// Tag(0x0009, 0x0102) + /// ); + /// ``` pub fn put_private_element( &mut self, group: GroupNumber, @@ -3731,6 +3797,16 @@ mod tests { PrimitiveValue::Str("1.0".to_string()), ) .unwrap(); + + let res = ds.put_private_element( + 0x0012, + "CREATOR 4", + 0x02, + VR::DS, + PrimitiveValue::Str("1.0".to_string()), + ); + assert_eq!(&res.err().unwrap().to_string(), "Group number must be odd, found 0x0012"); + assert_eq!( ds.private_element(0x0009, "CREATOR 1", 0x01) .unwrap() @@ -3755,4 +3831,19 @@ mod tests { Tag(0x0009, 0x1202) ); } + + #[test] + fn private_element_group_full(){ + let mut ds = InMemDicomObject::from_element_iter( + (0..=0x00FFu16).into_iter() + .map(|i| DataElement::new( + Tag(0x0009, i), + VR::LO, + PrimitiveValue::from("CREATOR 1"), + )) + .collect::>>() + ); + let res = ds.put_private_element(0x0009, "TEST", 0x01, VR::DS, PrimitiveValue::from("1.0")); + assert_eq!(res.err().unwrap().to_string(), "No space available in group 0x0009"); + } } From e7f29e4e6a63203b1d99eeff1ba406e56fe9c7fd Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Tue, 23 Apr 2024 10:39:29 -0500 Subject: [PATCH 08/11] MAIN: Run cargo fmt --- object/src/mem.rs | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 9f9d481f6..1711fc4cd 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -716,7 +716,7 @@ where } /// Get a private element from the dataset using the group number, creator and element number. - /// + /// /// For more info, see the [DICOM standard section on private elements](https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html) /// /// ## Example @@ -787,14 +787,14 @@ where /// Insert a private element into the dataset, replacing (and returning) any /// previous element of the same attribute. - /// + /// /// This function will find the next available private element block in the given /// group. If the creator already exists, the element will be added to the block /// already reserved for that creator. If it does not exist, then a new block - /// will be reserved for the creator in the specified group. - /// + /// will be reserved for the creator in the specified group. + /// /// For more info, see the [DICOM standard section on private elements](https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html) - /// + /// /// ## Example /// ``` /// # use dicom_core::{VR, PrimitiveValue, Tag, DataElement, header::Header}; @@ -3805,7 +3805,10 @@ mod tests { VR::DS, PrimitiveValue::Str("1.0".to_string()), ); - assert_eq!(&res.err().unwrap().to_string(), "Group number must be odd, found 0x0012"); + assert_eq!( + &res.err().unwrap().to_string(), + "Group number must be odd, found 0x0012" + ); assert_eq!( ds.private_element(0x0009, "CREATOR 1", 0x01) @@ -3833,17 +3836,19 @@ mod tests { } #[test] - fn private_element_group_full(){ + fn private_element_group_full() { let mut ds = InMemDicomObject::from_element_iter( - (0..=0x00FFu16).into_iter() - .map(|i| DataElement::new( - Tag(0x0009, i), - VR::LO, - PrimitiveValue::from("CREATOR 1"), - )) - .collect::>>() + (0..=0x00FFu16) + .into_iter() + .map(|i| { + DataElement::new(Tag(0x0009, i), VR::LO, PrimitiveValue::from("CREATOR 1")) + }) + .collect::>>(), ); let res = ds.put_private_element(0x0009, "TEST", 0x01, VR::DS, PrimitiveValue::from("1.0")); - assert_eq!(res.err().unwrap().to_string(), "No space available in group 0x0009"); + assert_eq!( + res.err().unwrap().to_string(), + "No space available in group 0x0009" + ); } } From 94bdd6996d44d83d550c4f385b70e7cc71754516 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Tue, 23 Apr 2024 11:24:10 -0500 Subject: [PATCH 09/11] MAIN: Use `?` in tests instead of `unwrap` --- object/src/mem.rs | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 1711fc4cd..9920c2f39 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -723,8 +723,8 @@ where /// /// ``` /// # use dicom_core::{VR, PrimitiveValue, Tag, DataElement}; - /// # use dicom_object::InMemDicomObject; - /// + /// # use dicom_object::{InMemDicomObject, PrivateElementError}; + /// # use std::error::Error; /// let mut ds = InMemDicomObject::from_element_iter(vec![ /// DataElement::new( /// Tag(0x0009, 0x0010), @@ -734,13 +734,12 @@ where /// DataElement::new(Tag(0x0009, 0x01001), VR::DS, PrimitiveValue::from("1.0")), /// ]); /// assert_eq!( - /// ds.private_element(0x0009, "CREATOR 1", 0x01) - /// .unwrap() + /// ds.private_element(0x0009, "CREATOR 1", 0x01)? /// .value() - /// .to_str() - /// .unwrap(), + /// .to_str()?, /// "1.0" /// ); + /// # Ok::<(), Box>(()) /// ``` pub fn private_element( &self, @@ -799,6 +798,7 @@ where /// ``` /// # use dicom_core::{VR, PrimitiveValue, Tag, DataElement, header::Header}; /// # use dicom_object::InMemDicomObject; + /// # use std::error::Error; /// let mut ds = InMemDicomObject::new_empty(); /// ds.put_private_element( /// 0x0009, @@ -806,23 +806,20 @@ where /// 0x02, /// VR::DS, /// PrimitiveValue::Str("1.0".to_string()), - /// ) - /// .unwrap(); + /// )?; /// assert_eq!( - /// ds.private_element(0x0009, "CREATOR 1", 0x02) - /// .unwrap() + /// ds.private_element(0x0009, "CREATOR 1", 0x02)? /// .value() - /// .to_str() - /// .unwrap(), + /// .to_str()?, /// "1.0" /// ); /// assert_eq!( - /// ds.private_element(0x0009, "CREATOR 1", 0x02) - /// .unwrap() + /// ds.private_element(0x0009, "CREATOR 1", 0x02)? /// .header() /// .tag(), /// Tag(0x0009, 0x0102) /// ); + /// # Ok::<(), Box>(()) /// ``` pub fn put_private_element( &mut self, From 3b5eb363958aa9557902a059e3fc0916f4584698 Mon Sep 17 00:00:00 2001 From: Eduardo Pinho Date: Thu, 25 Apr 2024 10:18:30 +0100 Subject: [PATCH 10/11] [object] Document PrivateElementError --- object/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/object/src/lib.rs b/object/src/lib.rs index 9cf3d4219..cffe3f48c 100644 --- a/object/src/lib.rs +++ b/object/src/lib.rs @@ -287,13 +287,17 @@ pub enum WriteError { WriteUnsupportedTransferSyntax { uid: String, backtrace: Backtrace }, } +/// An error which may occur during private element look-up or insertion #[derive(Debug, Snafu)] #[non_exhaustive] pub enum PrivateElementError { + /// Group number must be odd #[snafu(display("Group number must be odd, found {:#06x}", group))] InvalidGroup { group: GroupNumber }, + /// Private creator not found in group #[snafu(display("Private creator {} not found in group {:#06x}", creator, group))] PrivateCreatorNotFound { creator: String, group: GroupNumber }, + /// Element not found in group #[snafu(display( "Private Creator {} found in group {:#06x}, but elem {:#06x} not found", creator, @@ -305,6 +309,7 @@ pub enum PrivateElementError { group: GroupNumber, elem: u8, }, + /// No space available for more private elements in the group #[snafu(display("No space available in group {:#06x}", group))] NoSpace { group: GroupNumber }, } From e13733c3807656a3668253a8e164151bc6f50599 Mon Sep 17 00:00:00 2001 From: Eduardo Pinho Date: Thu, 25 Apr 2024 10:31:53 +0100 Subject: [PATCH 11/11] [object] Tweak documentation of new methods - tweak phrasing and expand error conditions - adjust doctests to be more idiomatic --- object/src/mem.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 9920c2f39..80eaa8a2b 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -717,7 +717,13 @@ where /// Get a private element from the dataset using the group number, creator and element number. /// - /// For more info, see the [DICOM standard section on private elements](https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html) + /// An error is raised when the group number is not odd, + /// the private creator is not found in the group, + /// or the private element is not found. + /// + /// For more info, see the [DICOM standard section on private elements][1]. + /// + /// [1]: https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html /// /// ## Example /// @@ -725,13 +731,13 @@ where /// # use dicom_core::{VR, PrimitiveValue, Tag, DataElement}; /// # use dicom_object::{InMemDicomObject, PrivateElementError}; /// # use std::error::Error; - /// let mut ds = InMemDicomObject::from_element_iter(vec![ + /// let mut ds = InMemDicomObject::from_element_iter([ /// DataElement::new( /// Tag(0x0009, 0x0010), /// VR::LO, /// PrimitiveValue::from("CREATOR 1"), /// ), - /// DataElement::new(Tag(0x0009, 0x01001), VR::DS, PrimitiveValue::from("1.0")), + /// DataElement::new(Tag(0x0009, 0x01001), VR::DS, "1.0"), /// ]); /// assert_eq!( /// ds.private_element(0x0009, "CREATOR 1", 0x01)? @@ -791,8 +797,11 @@ where /// group. If the creator already exists, the element will be added to the block /// already reserved for that creator. If it does not exist, then a new block /// will be reserved for the creator in the specified group. + /// An error is returned if there is no space left in the group. /// - /// For more info, see the [DICOM standard section on private elements](https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html) + /// For more info, see the [DICOM standard section on private elements][1]. + /// + /// [1]: https://dicom.nema.org/medical/dicom/2024a/output/chtml/part05/sect_7.8.html /// /// ## Example /// ``` @@ -805,7 +814,7 @@ where /// "CREATOR 1", /// 0x02, /// VR::DS, - /// PrimitiveValue::Str("1.0".to_string()), + /// PrimitiveValue::from("1.0"), /// )?; /// assert_eq!( /// ds.private_element(0x0009, "CREATOR 1", 0x02)?