-
Notifications
You must be signed in to change notification settings - Fork 261
DOC: Dicom doc. improvement #910
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.
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
Conversation
Better readability
Codecov Report
@@ Coverage Diff @@
## master #910 +/- ##
=======================================
Coverage 91.73% 91.73%
=======================================
Files 97 97
Lines 12359 12359
Branches 2177 2177
=======================================
Hits 11338 11338
Misses 684 684
Partials 337 337 Continue to review full report at Codecov.
|
@@ -243,7 +243,11 @@ Where: | |||
For later convenience we also define values useful for 3D volumes: | |||
|
|||
* $s$ : slice index to the slice plane. The first slice index is zero. | |||
* $\Delta{s}$ - spacing in mm between slices. | |||
* $\Delta{s}$ - Spacing in mm between slices, given by the Spacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dear - I see now that it is more complicated than I thought when I wrote the original - https://stackoverflow.com/questions/14930222/how-to-calculate-space-between-dicom-slices-for-mpr - what do you think we should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the method with Image Position (Patient) is more general, as some dicoms may miss the Spacing Between Slices attribute.
If both attributes appear, then filling the third column in the affine is possible with either:
- Image Position (Patient) of multiple sorted slices
- Spacing Between Slices + the normal n from Image Orientation (Patient).
And they are equivalent (at least are expected to be).
Method 1 is described thoroughly in the doc:
https://nipy.org/nibabel/dicom/dicom_orientation.html#d-affine-formulae
Method 2 may be helpful in some special case: you have just one slice (N=1) but still want the third column of the 4x4 affine matrix. It can be achieved with:
- The cross product of Image Orientation (Patient) = n
- Spacing Between Slices = Δs
In summary, it does not contradict anything but worth mentioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I think the question is whether to add the explicit mention of the DICOM Spacing Between Slices attribute here - because it looks like it is a pretty unreliable method of getting the thing we want, which is the distance between the center of one slice and the center of the next: https://groups.google.com/forum/#!msg/comp.protocols.dicom/gyvsgAj4y6o/KOjJywTWFlAJ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the bold part here can be moved to some footnote:
$\Delta{s}$ - Spacing in mm between slices, given by the Spacing ...
However, from my experience when this tag is there - it is correct, i.e. it agrees with the current method.
David A. Clunie is a reliable source, but it was 20 years ago. I think that today most of the manufacturers comply with the definition in the standard, which is exactly what we want:
Spacing Between Slices | (0018,0088) | 3 | Spacing between adjacent slices, in mm. The spacing is measured from the center-to-center of each slice, and if present shall not be negative.
From:
http://dicom.nema.org/MEDICAL/dicom/2015b/output/chtml/part03/sect_C.7.6.16.2.html#table_C.7.6.16-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable - maybe with some discussion like here so people know to be a little careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree about the rest of the changes though.
Related to N=1, A_{single}
Thanks for persisting. Looks good. |
Hi, your package and docs are great!
I found some possible improvements for the dicom documentation, as suggested in this PR: