Skip to content

ENH: return sorted file list when reading DICOM series #584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 29, 2022
Merged

ENH: return sorted file list when reading DICOM series #584

merged 1 commit into from
Jun 29, 2022

Conversation

jadh4v
Copy link
Member

@jadh4v jadh4v commented Jun 21, 2022 •
edited
Loading

The functions readImageDICOMFileSeries and readImageDICOMArrayBufferSeries
have an additional side effect that it sorts / orders the files
(particularly DICOM files) based on a pre-defined logic in gdcm.
However, this information is not relayed to the caller in any form.

Add Array<string> as the ordered list of filenames as additional output.

Addresses: #576

@jadh4v
Copy link
Member Author

jadh4v commented Jun 21, 2022 •
edited
Loading

@thewtex @floryst @sankhesh
I am not fully certain of the API changes are a good move. We could also return this info in any other way like using Image object's metaData property. Unfortunately, the Instance UIDs for each file/slice are not available in the reader's pipeline unless we explicitly read the DICOM Tags. That is why I stuck with the filenames as the returned output. And the filenames don't seem suitable as metadata of the Image.
cc: @FezVrasta

@thewtex
Copy link
Member

thewtex commented Jun 21, 2022

Hi @jadh4v thanks for working on this!

Why not add these as as metadata entries on the Image? Perhaps add both a Filenames and InstanceUIDs entry.

@jadh4v jadh4v force-pushed the ENH-return-filenames-dicom-reader branch 2 times, most recently from a28eba1 to db75d9d Compare June 22, 2022 22:15
@jadh4v
Copy link
Member Author

jadh4v commented Jun 23, 2022

I have added the returned ordered list of files as metadata to the output Image object rather than returning it as additional output for readImageDICOMArrayBufferSeries.

@jadh4v
Copy link
Member Author

jadh4v commented Jun 27, 2022

@floryst @sankhesh please review

@jadh4v
ENH: return sorted file list when reading DICOM series
59c2b62 
The functions readImageDICOMFileSeries and readImageDICOMArrayBufferSeries
have an additional side effect that it sorts / orders the files
(particularly DICOM files) based on a pre-defined logic in gdcm.
However, this information is not related to the caller in any form.

Add Array<string> as the ordered list of filenames as additional output.
@jadh4v jadh4v force-pushed the ENH-return-filenames-dicom-reader branch from db75d9d to 59c2b62 Compare June 28, 2022 20:30
floryst
floryst approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@floryst floryst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @thewtex if you are good with this, we can merge.

@floryst floryst merged commit 99c2ff1 into InsightSoftwareConsortium:master Jun 29, 2022
floryst added a commit that referenced this pull request Jul 1, 2022
@floryst
revert(#584): undo commit with invalid message
1dabba9 
This reverts commit 59c2b62.
floryst added a commit that referenced this pull request Jul 1, 2022
@floryst
Merge pull request #586 from InsightSoftwareConsortium/revert-584
d04489f 
revert(#584): undo commit with invalid message
@jadh4v jadh4v mentioned this pull request Jul 1, 2022
Merged
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

🎉 This PR is included in version 1.0.0-b.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floryst floryst floryst approved these changes

Assignees
No one assigned
Labels
released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jadh4v @thewtex @floryst