Page MenuHomePhabricator

Last time step of 3D+t data with arbitrary time geometry not selectable by image navigator spinbox
Closed, ResolvedPublic

Description

Steps to reproduce:

  • Open MITK-Data/3D+t-Heart
  • Choose file reader: MITK DICOM Reader v2 (autoselect)
  • Use spin box (arrow up) in Image Navigator to navigate to last time step

--> Slider alternates between time step 0 and 1 (data set has 3 time steps)

If you use the handle of the time step slider directly, the handle can be moved to time step 2, but the image of time step 0 is shown!
Exporting the image to .nrrd does not solve the issue.
However, that issue did not occur with other 3d+t .nrrd images.

Revisions and Commits

Event Timeline

eisenman triaged this task as Normal priority.May 11 2018, 2:15 PM
eisenman created this task.
kislinsk added subscribers: hentsch, floca.

I converted the image to NRRD. This is the header without any DICOM tags:

type: double
dimension: 4
space dimension: 4
sizes: 128 128 39 3
space directions: (1.953125,0,0,0) (0,1.953125,0,0) (0,0,3,0) (0,0,-121.98996734619141,1)
kinds: domain domain domain domain
endian: little
encoding: gzip
space origin: (-121.98996734619141,-136.40467834472656,-48.722412109375,0)
org_mitk_timegeometry_timepoints:=0 103 160 160
org_mitk_timegeometry_type:=ArbitraryTimeGeometry

Also there are several things a bit strange in my opinion, the actual reason for the spin box value to jump back from 2 to 1 as soon as it is increased is the value of org_mitk_timegeometry_timepoints:

org_mitk_timegeometry_timepoints:=0 103 160 160

Note that there are four values even though the image has only three time steps. The spin box issue disappears by either deleting the last timepoint or by making it different to the one before.

I remember that @hentsch and @floca did something with the last timepoint of arbitrary time geometries not long ago. Maybe they have an idea.

kislinsk renamed this task from Last time step of 3D+t data not selectable when loaded with default DICOM reader to Last time step of 3D+t data with arbitrary time geometry not selectable by image navigator spinbox.May 15 2018, 10:54 AM

Seems like I misunderstood the representation of org_mitk_timegeometry_timepoints. It needs four values for three time steps (n+1). When reducing the values to three values there's a warning in the terminal:

12.69 core.mod.core.imgIo ERROR: Stored timepoints (2) and size of image time dimension (3) do not match. Switch to ProportionalTimeGeometry fallback

So the actual fix is to change the last value to like 170.

The original timesteps are (ArbitraryTimeGeometry):

min TimeBounds: 
 Step 0: 0 ms
 Step 1: 103 ms
 Step 2: 160 ms

max TimeBounds: 
 Step 0: 103 ms
 Step 1: 160 ms
 Step 2: 160 ms

in another dataset, the navigation spinbox works. Following timesteps:

min TimeBounds: 
        ...
        Step 31: 700 ms

       max TimeBounds: 
       ...
        Step 31: 720 ms

So, I assume that min TimeBounds and max TimeBounds are compared.
Still, the org_mitk_timegeometry_timepoints after converting to nrrd is odd.

I looked around in QmitkImageNavigatorView and QmitkSliderNavigatorWidget, but I didn't stumble over anything suspicious.

One root to this behavior is the fact, that the current time geometries did not support "gaps" between time points and we didn't dare to introduce such a feature without very carefull thinking and evaluation, because it could have a tons of sideeffects and broken dependencies.

This explains e.g.

Still, the org_mitk_timegeometry_timepoints after converting to nrrd is odd.
[...] org_mitk_timegeometry_timepoints:=0 103 160 160

To currently represent a time geometry without gaps and with n time points, we need n+1 values. The first value is the min time bound of the first time point, the next n values are the maximum bounds of all time points.
In Dicom we have a lot of data sets, where a whole volume is encoded to cover just one point in time. So the original information read from DICOM would be [min bound j, max bound j]:
[0,0] [103,103] [160, 160]

Because MITK currently allows no gaps, it is converted in:
[0,103] [103,160] [160, 160]
And underlined values are stored in nrrd to define the geometry.

So basically in the final step we represent whats in the DICOM. That was also the reason why we do not artifically change the last max bound to something like +inf or assime the same duration like the preceeding time step. We did not want to alter the data because that could have implication we can not forsee now.

My bet is that the widget have problems with the fact thet the last time step is infitesimal small. May be just a ">" check and not a ">=" check on the minimal bound of a time step to check if its within the time step.
So basically I think currently we should check that all busiiness code checks time bounds of geometry time steps like a closed open set: [min bound, max bound). Meaning that min bound is within the coverage of a time step and max bound itself is not within the range of the time step

So basically I think currently we should check that all busiiness code checks time bounds of geometry time steps like a closed open set: [min bound, max bound). Meaning that min bound is within the coverage of a time step and max bound itself is not within the range of the time step

This is at least how it is internally implemented in the ArbitraryTimeGeometry if you request the time step for a certain time point.

Any more ideas on this? I stumbled across this while investigating another DICOM problem.

This does not happen with the new mxn multi widget. Maybe connected to the render windows being synchronized / plane nodes when using the StdMultiWidget?

Edit: The mxn multi widget does not display different time points at all. So changing the time slider / time spin box does not affect the visualization of the data.

I think the problem is here: https://phabricator.mitk.org/source/mitk/browse/master/Modules/Core/src/DataManagement/mitkArbitraryTimeGeometry.cpp$124
The m_MaximumTimePoints for the 3D+t-Heart image is [103, 160, 160]. The last timePoint argument is 160. So in the first iteration if (timePoint < *pos) will be false. Next two iterations it will be false as well since 160 is not less than 160. This will lead to result being 0 at the end of the function.
Normally the m_MaximumTimePoints for another time-image is something like [41, 46, 51] and the timePoint argument will be 46 for the last timestep.
This could be solved by using something like this:

mitk::TimeStepType mitk::ArbitraryTimeGeometry::TimePointToTimeStep(TimePointType timePoint) const
{
  mitk::TimeStepType result = 0;

  if (timePoint >= GetMinimumTimePoint())
  {
    for (auto pos = m_MaximumTimePoints.cbegin(); pos != m_MaximumTimePoints.cend(); ++pos)
    {
      if (timePoint < *pos)
      {
        break;
      }
      result++;
    }
  }

  return result;
}

So we are increase result continuously. However, I'm not sure about any side effects that might come with this (e.g. invalid timePoint, that should resolve to timestep 0 instead of the last timestep).

Using the modified function T26276 could be solved.

@kalali: Sharp eyes and good work. As far as I can see this is a proper solution to fix a state I had not covered in my implementation. The handling of invalid timepoints is still okay. As the documentation only guaranties that negative invalids will be 0. For all others there are no garanties and developers should use GetTimeBounds() to check wether they request legal time points or not.

If you add the fix, please also extend the unit test for ArbitraryTimeGeometry to cover such a scenario as well. (So the new test should faile with the old code, but work with your fix.) Thanks.

kalali added a revision: Restricted Differential Revision.Aug 30 2019, 2:38 PM
kalali claimed this task.

So basically I think currently we should check that all busiiness code checks time bounds of geometry time steps like a closed open set: [min bound, max bound). Meaning that min bound is within the coverage of a time step and max bound itself is not within the range of the time step

This is at least how it is internally implemented in the ArbitraryTimeGeometry if you request the time step for a certain time point.

In hindside I would argument differently. It is correct that the implementation of ArbitraryTimeGeometry and PropertionalTimeGeometry implement it that way. But the documentation of TimeGeometry defines it as closed interval and this is also how the spatial bound checks are implemented. It makes sense to keep everything in the same behavior. See also T28262 for that reflection.