Skip to content

Dicom multi volume series#31

Merged
floryst merged 7 commits intomasterfrom
dicom-multi-volume-series
Apr 15, 2021
Merged

Dicom multi volume series#31
floryst merged 7 commits intomasterfrom
dicom-multi-volume-series

Conversation

@floryst
Copy link
Copy Markdown
Contributor

@floryst floryst commented Mar 8, 2021

This adds some handling for series containing multiple volumes.

This PR is marked as a draft, as it depends on the oriented-views branch for correct viewing of the datasets.

FYI @agirault

@floryst floryst force-pushed the dicom-multi-volume-series branch from 2c03462 to cd05376 Compare March 8, 2021 18:31
agirault
agirault reviewed Mar 9, 2021
View reviewed changes
Copy link
Copy Markdown

@agirault agirault left a comment

Choose a reason for hiding this comment

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

Wow, cool work. A couple of questions and suggestions for naming.

Comment thread src/io/itk-dicom/base64.hpp Outdated
Comment thread src/io/itk-dicom/dicom.cpp Outdated
Comment thread src/io/itk-dicom/dicom.cpp Outdated
Comment thread src/io/itk-dicom/dicom.cpp Outdated
Comment thread src/io/itk-dicom/dicom.cpp Outdated
Comment thread src/io/itk-dicom/dicom.cpp Outdated
Comment thread src/io/itk-dicom/dicom.cpp
Comment thread src/io/itk-dicom/dicom.cpp
Comment thread src/store/dicom.js Outdated
Comment thread src/components/PatientBrowser.vue Outdated
@floryst
Copy link
Copy Markdown
Contributor Author

floryst commented Mar 9, 2021

We have a naming issue at hand. For identifying various aspects of the hierarchy, we previously had:

  • PatientID: DICOM patientID
  • StudyInstanceUID: DICOM study uid
  • SeriesInstanceUID: DICOM series uid

Since we now have distinct datasets coming out of a single series, we need a new UID name. Heer are suome suggestions:

  • VolumeUID
  • ObjectUID
  • DatasetUID

Which one do you prefer? @agirault

@agirault
Copy link
Copy Markdown

agirault commented Mar 9, 2021

Object would not be good. On top of being too generic, it is already defined in DICOM as an instance of an SOP class/IOD. i.e.: each separate files are already defined as instances or objects.

The PR itself is called "multi-volume series", so I think Volume is at least an easy-to-understand option. It's also the one used by 3D Slicer. The only problem that I can think of is that it's not exactly a volume if it's only one scan, or if it's 2d images + time. So I guess, Volume or Dataset.

@floryst
Copy link
Copy Markdown
Contributor Author

floryst commented Mar 9, 2021 via email •
edited by agirault
Loading

@agirault
Copy link
Copy Markdown

agirault commented Mar 9, 2021 •
edited
Loading

I think Volume makes sense, and the consistency with 3DSlicer is good as well. Oh, and technically, should it be ID and not UID? Not sure we can ensure it's actually unique... but that's nitpicking.

@floryst floryst force-pushed the dicom-multi-volume-series branch from cd05376 to ecde06a Compare March 15, 2021 16:03
@floryst
Copy link
Copy Markdown
Contributor Author

floryst commented Mar 15, 2021 •
edited
Loading

Before this gets approved/merged, I will submit a PR to this branch containing a refactor from operating on series to operating on volumes, since the whole premise of this PR is to support handling multiple volumes within a single series. I'm separating those changes into another PR because the changes are extensive and I want to review that separately.

floryst added 4 commits March 16, 2021 09:51
@floryst
feat(dicom): Support multi-volume series
51df118 
This adds support for processing a series that contains multiple
volumes, distinguishable by their orientation.
Due to potential errors in floating point, each series slice is binned
according to some tolerance on the orientation.
The orientation-based restriction is performed after ITK-GDCM performs
its restrictions.

We do not use AddSeriesRestriction because it does not handle the case
where two orientations are almost equal, but are off by some small
epsilon.

To correctly identify the volumes within a single series, we no longer
can rely on SeriesInstanceUID.
Instead, we use our custom-generated "series UID" to identify unique
objects.
In this case, each unique object corresponds to a volume.
@floryst
fix(VtkView): Clip long annotation text
293b7d2
@floryst
chore(dicom): Allow for local builds with fs
e478d85 
In my GCC version (e.g. 7.5.0), the filesystem header is under
"experimental/filesystem".
The long-term solution would be to have CMake inject a macro variable
indicating if we should draw from <filesystem> or
<experimental/filesystem>.
@floryst
chore(scripts): Add dicom debug build command
1d7b49c
@floryst floryst force-pushed the dicom-multi-volume-series branch from ecde06a to 142ac6e Compare March 16, 2021 13:53
@floryst floryst changed the base branch from oriented-views to master March 22, 2021 14:58
floryst added 3 commits March 24, 2021 11:40
@floryst
refactor: Switch to volumes as a dataset unit
1b412e6 
Because a series can have multiple volumes, it is more ideal to refer to
volumes rather than series when displaying individual images.
@floryst
docs: Fix up docstrings
9a14f76
@floryst
chore(dicom): Update prebuilts
c84d845
@floryst floryst force-pushed the dicom-multi-volume-series branch from 041b7c3 to c84d845 Compare March 24, 2021 15:43
@floryst floryst merged commit e90323e into master Apr 15, 2021
@floryst floryst deleted the dicom-multi-volume-series branch April 15, 2021 18:08
PaulHax pushed a commit to PaulHax/VolView that referenced this pull request Apr 24, 2025
@floryst
Merge pull request Kitware#31 from KitwareMedical/dicom-multi-volume-…
e03a948 
…series

Dicom multi volume series
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@agirault agirault agirault left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@floryst @agirault