Implement DICOMDIR#669
Conversation
|
@Enet4 I am looking into how to extract the DICOM hierarchy from the DICOMDIR file.
Is there already an API to read information by offsets in dicom-rs? If not can you point me to some part in the code where the offsets are read currently? |
The mid-level data set reader API in
Looking forward to your thoughts on these approaches, though I'm feeling inclined to option c) right now. |
Enet4
left a comment
Enet4
left a comment
There was a problem hiding this comment.
Thank you for the initiative! Though this is surely a work in progress, I took the liberty of leaving some notes and suggestions inline.
| pub fn get_referenced_file_ids(&self) -> Vec<String> { | ||
| let directory_record_sequence = self.file | ||
| .element(tags::DIRECTORY_RECORD_SEQUENCE) | ||
| .expect("could not get DIRECTORY_RECORD_SEQUENCE") |
There was a problem hiding this comment.
I suspect this is just temporary code, but all of these expects should become recoverable errors before merging. It is fine to create a new error type specifically for DicomDir methods.
An alternative approach to this is pre-validate and transform all data as it is read from the file, so that these methods become pure getters.
| }) | ||
| } | ||
|
|
||
| pub fn get_referenced_file_ids(&self) -> Vec<String> { |
There was a problem hiding this comment.
In this case we can make do without the get_ prefix (at least per my interpretation of C-GETTER, as this is something like a fallible getter).
| pub fn get_referenced_file_ids(&self) -> Vec<String> { | |
| pub fn referenced_file_ids(&self) -> Vec<String> { |
| fn test_dicom_dir() { | ||
| let current_dir = env::current_dir().unwrap(); | ||
| println!("Current directory: {}", current_dir.display()); | ||
| let path = "src/dicomdirtests/DICOMDIR"; |
There was a problem hiding this comment.
We need a better strategy for holding test assets. If they sit alongside src, it'll be harder to exclude them from packages.
Even better would be introducing this data set to dicom-test-files so that they stay in a separate repository and are only fetched when needed.
| @@ -0,0 +1,69 @@ | |||
| use std::path::Path; | |||
There was a problem hiding this comment.
As a public module, some crate-level documentation would be nice to have. An example: https://docs.rs/dicom-object/latest/dicom_object/collector/index.html
| pub type Result<T, E = ReadError> = std::result::Result<T, E>; | ||
|
|
||
| impl DicomDir { | ||
| pub fn new<P>(path: P) -> Result<DicomDir> |
There was a problem hiding this comment.
For consistency, either open_file or open_dicomdir would be suitable function names for this one.
|
Hello there, Editing / creating at this time is not bug free, which is strongly coupled with #645 . Unless this is fixed, the creation of a DICOMDIR will suffer from length calculation / recalculation bugs, leading to incorrect offset values. |
|
@jmlaka interesting! Thanks for the info. |
@Cryt1c sorry for the late reply as I haven't been very active lately. Here I made this public repo for you: It is updated to compile against the newest dicom 0.9.0 It was originally meant to support read, write, update ... The main logic was copied from PyDicom code. The binary lets you test the capabilities. It has a lot of debugging output. Included /test_files come from the PyDicom project itself Read capability works flawlessly and can be integrated even into the Dicom library (after getting rid of everything else) creating new dicomdirs from a directory of dicom instances works (I Think) Now so sure about updating the datasets. As you can see, I you want to do anything more beyond simple parsing of a DICOMDIR file, you have to If you extract the read part of the code (just parsing the file paths) it is pretty lightweight stuff. |
3 participants
#184
Done: