Page MenuHomePhabricator

TimeGeometry inconsistent documentation and implementation of time bounds and checks
Closed, ResolvedPublic

Description

Status:

  1. Currently the documentation of TimeGeometry states that GetMinumumTimePoint() and GetMaximumTimePoint() are the valid bounds. So time geometry bounds for the whole time geometry and its steps are closed intervals [GetMinumumTimePoint, GetMaximumTimePoint]. That makes sense as it follows the principle of minimal surprise, because the itk::BoundingBox (used for the spatial bounds, does it the same way.
  2. The implementation of PropertionalTimeGeometry instead handles the bounds as a semiclosed interval [GetMinumumTimePoint, GetMaximumTimePoint). So MaximumTimePoint is the first first point not inside the time bounds
  3. The implementation of ArbitraryTimeGeometry instead handles the bounds mostly as a semiclosed interval [GetMinumumTimePoint, GetMaximumTimePoint). So MaximumTimePoint is the first first point not inside the time bounds. The only exception is for colapse final time points as the happen in DICOM images.

This problem becomes obvious now e.g. in T27883 when you work in the last time step of a collapsed ArbitraryTimeGeometry (basically any dynamic DICOM image). Then we have different interval checks and there for inconsistencies and errors.

Discussion point:
a. A work arround could be to change the handling of collapsed final time steps in ArbitraryTimeGeometry and define that this indicates a coverage up to positive max. So if a final timestep as no defined extend and would therefore infitesimal small, we treat him as he covers the whole rest of the time.
It should work, but not tested yet and it might have other implications that I currently do not see. We had this discussion multiple time in the past and so far we always were reluctant to go that way: a) because we would alter/generate information we do not have; b) because we did not know if this might not break any code. (see e.g. discussion in T24766 i.a.)

b. Even though a) might make sense, I think we should take care of this inconsistency. The only question is, if it should/must be before the release. I would depend it on the fact if the work arround a) is not just a work arround but we always want to handle it that way. But the arguments we had against a) still hold I think. And we cannot completly forsee the implications for both options. So it is between a rock and a hard place... 😒

IMPORTANT: If this issue is handled the workarround introduced for/by T27883 have to be removed. To find the workarrounds search for the following text in the code base: "Workarround T27883."

Event Timeline

floca triaged this task as Unbreak Now! priority.Jan 31 2021, 5:16 PM
floca created this task.
floca renamed this task from TimeGeometry inconstent documentation and implmentation of time bounds and checks to TimeGeometry inconsistent documentation and implmentation of time bounds and checks.Jan 31 2021, 5:19 PM

Why is the last timestep collapsed in the first place? Without having any context knowledge it seems to be the actual issue for me and makes not much sense as from looking at the values, it simply has a duration of 0. Is it really a flaw of DICOM or do we just come up with these values based on some other DICOM tags?

If we do not have any durations and just came up with the durations based on the deltas between time steps, we may want to also consider the following strategies:

  • If all time steps have the same distance from each other, set the duration of the last time step accordingly
  • If time steps have arbitrary durations, follow the MITK default for other cases and make it at least 1ms.

Why is the last timestep collapsed in the first place?

Because we have no information about the extend of the last time point... because, well, it is the last.

Without having any context knowledge it seems to be the actual issue for me
and makes not much sense as from looking at the values, it simply has a duration of 0.

Even if we remove the collapsed time step, we still have the core problem of this task: 1) documentation and implementation of time geometries do not match; 2) max time bounds have a different semantic than spatial max bounds (violated expectations!)
So I think it should be covered at some point (not saying it has to befor the release).

Is it really a flaw of DICOM or do we just come up with these values based on some other DICOM tags?

You can see it as a flaw, but only in part in DICOM but also in the vendor machines. It is just a fact that if you use single frame images to encode dynamic MRs and CTs, there is no official tag to store the frame duration and allmost all of the dynamic data we recieve is still of this kind. So you know that one frame ends as soon as you know when the next starts. And, 🎺tata, the last one has no next. So you do not know when it ends. This is why we have a colapsed final time step with all DICOM dynamic series loaded.

If we do not have any durations and just came up with the durations based on the deltas between time steps, we may want to also consider the following strategies:

  • If all time steps have the same distance from each other, set the duration of the last time step accordingly
  • If time steps have arbitrary durations, follow the MITK default for other cases and make it at least 1ms.

I think it is a question of philosophy. The reason why I always was and am hesitant to make up some duration, is that it is made up. We don't know it and we changing the information provided to us. We had this discussion several times in the past. And as for the dynamic modeling (e.g. perfusion) we only need to know when the frames start, it seem not right to make up things.
If the majority want's to go that way, OK.
But if I have to make something up, I would definetly picking a value that is also in hindside easier to identify as artfically added. E.g. std::numeric_traits::max or +inf.

But I think we are potentially pointing the finger on the wrong one. If PropertionalTimeGeometry would implement its bound checks as documented (as closed interval) then we wouldn't have problems (mentioned by you in T27883) in the first place, because last frame max bounds would also be valid for the single frame segmentation.

If we do not have any durations and just came up with the durations based on the deltas between time steps, we may want to also consider the following strategies:

  • If all time steps have the same distance from each other, set the duration of the last time step accordingly
  • If time steps have arbitrary durations, follow the MITK default for other cases and make it at least 1ms.

I think it is a question of philosophy. The reason why I always was and am hesitant to make up some duration, is that it is made up. We don't know it and we changing the information provided to us. We had this discussion several times in the past. And as for the dynamic modeling (e.g. perfusion) we only need to know when the frames start, it seem not right to make up things.
If the majority want's to go that way, OK.
But if I have to make something up, I would definetly picking a value that is also in hindside easier to identify as artfically added. E.g. std::numeric_traits::max or +inf.

It is philosophical indeed, but how is any of these strategies worse than creating a time step with a duration of 0? The correct behavior in general to handle this without writing edge cases would be to not show this time step at all as it does not exist at any time so this cannot be a solution in my opinion.

I think 1ms is not that random BTW in the context of MITK as it is exactly what we are doing as default for geometries for ages (and can be still seen in ProportionalGeometry). So while not perfect, it would be at least in line with everything else without changing existing behavior when redefining interval bounds. I am also fine with a max value but then again it may spark other questions like why we do not do this for ProportionalGeometries as well and there I think it feels naturally wrong to have like n-1 time steps with a certain duration and then suddenly an infinite duration at time step n. Projecting this back to the arbitrary geometries it would cause similar effects that a dynamic image is not visible before its first time step in the global scene but at anytime after that.

There's no possibility for a general solution here without having options for the user (which they probably won't understand as it is super technical). And probably this is theoretical as well since not many users will find themselves in the position to have to care here. :-) So, whatever it is, 1ms or max, I vote for it in favor of 0 without the need of having to break existing interval conventions that probably have other side effects that we are not aware of.

It is philosophical indeed, but how is any of these strategies worse than creating a time step with a duration of 0?

We should keep in mind that the whole concept of duration is purely artefical. There is no duration defined. The only things we know are the very time points where they exist.
And we should differentiate between things we might do at the data level and at the UI/Interaction level. We do the same in the spatial domain. Here we just do not allow images that do not have a equidistant spacing, so we avoid that mess. There it was an option, because those cases are so rare. For time they are not rare, so we have to deal with it.
And I realy do not want to produce data with a magically added duration, if we store it. Because we do not know and in the end only assume it for nicer UI experience and because we are afraid of potential breaking.

The correct behavior in general to handle this without writing edge cases

It is only an edge case because TimeGeometries are not implemented correctly. If the were implemented as closed interval there would be no edge case at all. Because everything is valid and the last time step just tells you: "I am only valid on my very position (e.g. 160 ms). Not befor and afterwards."

I think 1ms is not that random BTW in the context of MITK as it is exactly what we are doing as default for geometries for ages (and can be still seen in ProportionalGeometry).

First, only because it is tradition, it does not have to be correct. ;) Second, it was wrong for images then. This is why we had to introduce the ArbitraryTimeGeometry in the first place.
And personaly I think it was quite random then. If some one would have specified that the unit would be seconds it would be 1 second.

And also PropertionalTimeGeometry does not use always 1. Currently static images loaded span non_positive_min to max.

Projecting this back to the arbitrary geometries it would cause similar effects that a dynamic image is not visible before its first time step in the global scene but at anytime after that.

Not so implausible to me. It is the same behavour of static images right now, which can be seen as 1-frame-dynamic images and are shown for ever. I think it is realy a matter of taste and I will not defend it with my life.

So while not perfect, it would be at least in line with everything else without changing existing behavior when redefining interval bounds.

Hm, it is not the first time we make breaking changes to our TimeGeometry, because we thought it is worth it.
And switching to a closed check, would have no practical impact on static images and none on dynamic images as long as people have used the public interface for checking validity, followed what documentation said or have not fiddled around with own consitency checks.

I vote for it in favor of 0 without the need of having to break existing interval conventions that probably have other side effects that we are not aware of.

I am not convinced here and I think we have two tasks/problems intermingled, which we should seperate (bounds of geometries, collapsed time bounds). I think we should have a discussion on that. Damn...

What ever we choose I am strongly against any option that will have us storing data with made up duration which will be consumed by other systems.

Okay, back to the beginning. I still see two options that do not feel like a workaround.

In the task description you say that all time geometries handle time bounds as semi-closed interval [...). Except for collapsed time steps and this is the only exception, right? On the other side, our minimum time resolution seems to be 1ms? That means, that if we want to get rid of the edge case while keeping semi-closed intervals, setting the last time step duration to the minimum duration would be a very valid solution without introducing new meta-data as you said durations are purely artificial/internal to MITK - since DICOM does not seem to save durations anyway, we do not magically introduce new artificial data to derived DICOM files. The only thing left here is the confusing naming of "MaximumTimePoint" which feels more like a back() instead of and end() when using a standard library analogy.

Second option is to redefine our time bounds to be a closed interval in all cases, right? That would also makes more sense with the term "MaximumTimePoint". Then we have to identify many code locations were we have to potentially change < to <= and make sure that we use max() instead of +inf() for indefinitely long timesteps (same for lowest() vs. -inf()).

It is philosophical indeed, but how is any of these strategies worse than creating a time step with a duration of 0?

We should keep in mind that the whole concept of duration is purely artefical. There is no duration defined. The only things we know are the very time points where they exist.

I'd like to know why we need a duration in the first place?

It is philosophical indeed, but how is any of these strategies worse than creating a time step with a duration of 0?

We should keep in mind that the whole concept of duration is purely artefical. There is no duration defined. The only things we know are the very time points where they exist.

I'd like to know why we need a duration in the first place?

So far we do not need it. And it is not stored. There are use cases in DICOM where the duration of a frame could be less or more then the distance in time between two frames (simelar to fact that slices may have a thickness different to the z spacing), but we currently do not care.
So duration comes implicetly in our workbench as "the time till the next frame is valid".

I'd like to know why we need a duration in the first place?

Since we are supporting multiple images in a single scene. These images can have different time bounds and their time steps can be discretized differently. So even if you have kind of matching dynamic images but one image specifies its acquisition times at 2, 12, 22ms seconds, and the other one at 0, 10, and 20ms, you would have a very sparse discrete time line and only see either no image or at maximum a single image, but never both at the same time. So it is a pretty solid and natural assumption to define timesteps to have a duration until the next time step of the same image, if not explicitly specified otherwise. However, we also have many images that do not have time bounds stored in their files at all (in particular static images) and we need to come up with something completely artificial. It is a pretty severe issue but luckily is not very present in 99% of workflows as users rarely need to have scenes with different dynamic images aligned perfectly or at least not matching 1:1 on a time step basis instead of true time for their purposes.

It would be overkill but to get things sorted out for everyone MITK is missing a complete sophisticated time-line component were you can arrange images and change their time properties just like every video editing software allows you to do for clips.

Since we are supporting multiple images in a single scene. These images can have different time bounds and their time steps can be discretized differently. So even if you have kind of matching dynamic images but one image specifies its acquisition times at 2, 12, 22ms seconds, and the other one at 0, 10, and 20ms, you would have a very sparse discrete time line and only see either no image or at maximum a single image, but never both at the same time. So it is a pretty solid and natural assumption to define timesteps to have a duration until the next time step of the same image, if not explicitly specified otherwise. However, we also have many images that do not have time bounds stored in their files at all (in particular static images) and we need to come up with something completely artificial. It is a pretty severe issue but luckily is not very present in 99% of workflows as users rarely need to have scenes with different dynamic images aligned perfectly or at least not matching 1:1 on a time step basis instead of true time for their purposes.

It would be overkill but to get things sorted out for everyone MITK is missing a complete sophisticated time-line component were you can arrange images and change their time properties just like every video editing software allows you to do for clips.

But this is purely done for UI purposes? Because only see either no image or at maximum a single image, but never both at the same time is actually the expected behavior with two images that don't share any common timestep.

Also: so it is a pretty solid and natural assumption to define timesteps to have a duration until the next time step of the same image: For me this is not a natural assumption. My natural assumption would be to align the images - as you mentioned - time step after time step so 0, 2, 10, 12, 20, 22 ms.
If we need to view multiple images at the same time without the images being recorded at the same time step I think this is the job of the UI (or it's controlling class) to do this (like working with <= and >= instead of coding everything in the base data or the like).
It would also maybe interesting for the user to be able to see the data one after another (according to the real acquisition times) or to have them overlapping. Overlapping in this sense means, that images can be viewed simultaneously if they overlap in the time frame between two time steps.

With the current situation and with the things you mentioned: How would I be able to view multiple images simultaneously with

  • Image A has acquisition times at 2,3,4,5
  • Image B has acquisition times at 2,4

In this scenario, Image A's times at 2 and 3 would overlap with the same Image B at acquisition time 2?

Maybe we shouldn't hijack this task for the discussion but I'd like to have a closer look at this issue.

In the task description you say that all time geometries handle time bounds as semi-closed interval [...). Except for collapsed time steps and this is the only exception, right? On the other side, our minimum time resolution seems to be 1ms? That means, that if we want to get rid of the edge case while keeping semi-closed intervals, setting the last time step duration to the minimum duration would be a very valid solution without introducing new meta-data as you said durations are purely artificial/internal to MITK - since DICOM does not seem to save durations anyway, we do not magically introduce new artificial data to derived DICOM files. The only thing left here is the confusing naming of "MaximumTimePoint" which feels more like a back() instead of and end() when using a standard library analogy.

Second option is to redefine our time bounds to be a closed interval in all cases, right? That would also makes more sense with the term "MaximumTimePoint". Then we have to identify many code locations were we have to potentially change < to <= and make sure that we use max() instead of +inf() for indefinitely long timesteps (same for lowest() vs. -inf()).

Funny thing I came to a very simelar conclusion last night (not the worset indicator.

I tend to use the first option (if it ArbitrarTimeGeometry::HasCollapsedFinalTimeStep()==true we behave as 1ms but do not store it or so) as a mitigation strategy for the release.
I think it should work without big side effects. But I have to have a look. And all other options seems to messy or not manageble in time.

The second option is what I would prefer at long term. As we (potentially) break things with this change. I would realy like to explictly break it and e.g. change the Interface and naming. E.g. remove the GetMaximumTimePoint() and GetTimeBounds() for dedicated time steps. I think this is what lead to the semiclosed interpretation in the first place. Because a closed interprestation would not make sense for a single time step, as one would have conflicts at the borders of each timestep.

Also: so it is a pretty solid and natural assumption to define timesteps to have a duration until the next time step of the same image: For me this is not a natural assumption. My natural assumption would be to align the images - as you mentioned - time step after time step so 0, 2, 10, 12, 20, 22 ms.

Here I second Stefan and think it is OK to see it as a valid practical assumption. I have perfusion and other dynamic series use cases in mind. I think it would just break user expectations if you say, "sorry I cannot show you the last frame in between time points, because officially it is associated with the very time point".
And we do the same in the spatial domaine all the time. We have no black gaps in our CT volumes with 10 mm z-spacing, but the slice is only defined at one position in world space coordinate. If the thickness (==duration) is not defined, we fill the gap with known content.

But this is purely done for UI purposes?

No. I would call it a use case or domaine specific expectation of period of validity. And in many cases the people expect that a dynamic series discretizes a continuum in time (as normal images do in the spatial domain) and that one measurment is valid until the next one is there.

If we need to view multiple images at the same time without the images being recorded at the same time step I think this is the job of the UI (or it's controlling class) to do this (like working with > > [...]
Maybe we shouldn't hijack this task for the discussion but I'd like to have a closer look at this issue.

Yes, it is a bit of hijacking ;). But it is an interesting thought never the less. I would propose to open a dedicated task to discuss that idea.

It would be overkill but to get things sorted out for everyone MITK is missing a complete sophisticated time-line component were you can arrange images and change their time properties just like every video editing software allows you to do for clips.

@kislinsk: Make it a task. :) Maybe it happens somewhen...

IMPORTANT: If this issue is handled the workarround introduced for/by T27883 have to be removed. To find the workarrounds search for the following text in the code base: "Workarround T27883."
kalali renamed this task from TimeGeometry inconsistent documentation and implmentation of time bounds and checks to TimeGeometry inconsistent documentation and implementation of time bounds and checks.Apr 7 2021, 11:24 AM
kislinsk lowered the priority of this task from Unbreak Now! to High.Jul 14 2021, 11:09 AM

We can assume that this is a time-intensive task and a few solutions were already proposed but we do not see how to resolve it completely at the moment with our available resources.

floca lowered the priority of this task from High to Normal.May 12 2022, 11:04 AM
floca edited projects, added MITK (v2022.10); removed MITK, Next Milestone.
kislinsk claimed this task.
kislinsk added a project: Moved to git.dkfz.de.

This task was closed here on Phabricator since it was migrated to GitLab. Please continue on GitLab.