DICOM: use FrameTimePointer to detect timelapse data #4196
Conversation
There was a problem hiding this comment.
There is likely a question of how far we want to push this initial timelapse support in DICOM.
I agree with #4167 (comment) as a good starting point i.e. update the reader to support the CineModule and check for Frame Increment Pointer tags pointing either to Frame Time or Frame Time Vector.
As shown by the repository tests and the required configuration changes, we seem to have a few sample images including public sample images with multiple time frames e.g.
US-MONO2-8-8x-execho.dcm, NM-MONO2-16-13x-heart.dcm or MR-MONO2-8-16x-heart.dcm. I'll review the samples linked from #4167 (comment) to see if there are other representative public samples that should be added to the curated QA repository.
A couple of inline questions. Possibly the most critical engineering decision is whether we want to also store the time offset directly under DicomTile. This is a bigger change and would probably end up as a requirement in the case where we want to support DICOM files with multiple timepoints and focal planes.
| @@ -689,6 +717,23 @@ else if (child.attribute == OPTICAL_PATH_DESCRIPTION) { | |||
| } | |||
| if (imagesPerFile == 0) imagesPerFile = 1; | |||
|
|
|||
| // pointer could be FrameTime or FrameTimeVector, or possibly something else? | |||
There was a problem hiding this comment.
Since these two tags are defined in
andshould we make this logic defensive and check the tag attribute before setting frameTime?
There was a problem hiding this comment.
My thinking here was to not assume that the pointer would necessarily be FRAME_TIME or FRAME_TIME_VECTOR. For NM data in particular, I think that assumption would not hold (see https://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_C.8.4.8.html#sect_C.8.4.8.1.1), but could also imagine a file that has FrameIncrementPointer pointing to a private tag.
There was a problem hiding this comment.
Completely agreed that the reader should not assume the tag will reference one of these two tags or even official tags.
Maybe my question was more specifically about how we would handle both scenarios:
- if
Frame Increment Pointerpoints toFrame TimeorFrame Time Vectorthen the logic should be updated as proposed here to detect the image as multi-T and optionally set the time metadata appropriately - if
Frame Increment Pointerpoints to another tag (or multiple tags), should the current reader logic be preserved i.e. use the Z dimension to aggregate the frames? Or would we still switch to using the T dimension similarly to what MinimalTiffReader does for multi-page TIFF without additional metadata?
| int timepoint = coords[2]; | ||
| if (isTimelapse()) { | ||
| z = coords[2]; | ||
| timepoint = coords[1]; |
There was a problem hiding this comment.
If I understand correctly, this inversion is related to the fact that the frame offsets are currently stored as DicomTile.zOffset.
Do you have a feeling about the cost vs value of updating DicomTile to also store tOffset instead?
|
|
||
| for (DicomTag t : tags) { | ||
| if (frameIncrementPointer.equals(t.attribute)) { | ||
| frameTime = t; |
There was a problem hiding this comment.
Two possible metadata follow-ups
FrameTimecould likely be mapped intoPixels.TimeIncrementFrameTimeVectorcould be mapped intoPlane.DeltaT
|
Conflicting PR. Removed from build BIOFORMATS-push#161. See the console output for more details.
--conflicts |
|
Some test failures that look to be due to this being excluded: https://merge-ci.openmicroscopy.org/jenkins/job/BIOFORMATS-test-folder/lastFailedBuild/consoleFull Excluding the config PR temporarily until this is back in the merge build will likely be enough to resolve the failures |
|
Conflicting PR. Removed from build BIOFORMATS-push#164. See the console output for more details.
--conflicts |
|
Conflicting PR. Removed from build BIOFORMATS-push#165. See the console output for more details.
--conflicts |
This is a potential replacement for #4167. As recommended in #4167 (comment), this uses the presence of a valid
FrameTimePointerto determine ifSizeTshould be set instead ofSizeZ.This changes the behavior of a few existing sample files, so a configuration PR is forthcoming.