[Insight-developers] ImageSpatialObject Bug0006340

[Rupert Brooks]

To answer my own comment:

> I'm going to try changing itk::Image to always return unit direction
> cosines.  Dont worry, i wont submit a change that huge without
> discussing here first.  But I will let everyone know how many tests it
> breaks.  This will take a few hours, i'll report back when the build
> completes.

Since it seems clear that the issue with itk::Image will be dealt with
at another time and place, im not going to pursue this much further.
Just to complete the conversation for future reference though, heres
what i learned.

- The Get/Set direction are in itk::ImageBase.  It is easy to change
the SetDirection() method here to do nothing, which then keeps the
direction as identity.  However, it is necessary to cut and paste the
contents of this method, to replace the Superclass::SetDirection in
itk::OrientedImage.

I must confess i did not run a full battery of tests, as once it was
clear this was not going to be the solution, there was other work i
wanted to move on to.  However, i ran the first 1000 or so tests.
About 10 failed - in each case this was due to various tests that
explicitly set the directions in an itk::Image and expected them to
stay there.  This includes itkBrains2MaskTest, itkDirCosinesTest (part
of Nifti) and many of the itkImageIODirection tests.

So - it appears that it was an explicit design decision to have
itk::Image report direction cosine information, because it is being
explicitly tested for.  It seems primarily necessary for the case of
loading, and then saving an image, making sure not to lose its
directions.

Ok - we can probably leave that issue for another day.  As for the bug
that started it all, i have a patch thats running through an
experimental build and tests now.  Assuming it passes, i'll check it
in later today.

Cheers
Rupert

On Wed, Aug 6, 2008 at 8:24 AM, Rupert Brooks <rupe.brooks at gmail.com> wrote:
> I decided to investigate a bit further - I wondered if the problem
> could easily occur in practice.  I found that it can.
>
> I wrote a little code to load an image and print its direction info
> with both itk::Image, and itk::OrientedImage.  In both cases the
> direction info is correct, and non-identity.  To be clear - at least
> the MetaImageIO loads direction info and puts it into an ITK image,
> which would lead directly to an image that would create the problem i
> described.  So it would be very easy for a user to accidentally do
> this, leading to mysterious, occasional failures.
>
> I'm going to try changing itk::Image to always return unit direction
> cosines.  Dont worry, i wont submit a change that huge without
> discussing here first.  But I will let everyone know how many tests it
> breaks.  This will take a few hours, i'll report back when the build
> completes.
>
> By the way - I dont have particular feelings about a solution for this
> problem.  I use orientation in all my code, so the problem will never
> affect my work.  But i guess when you adopt a stray bug, like a stray
> puppy, you end up with all its consequences.   Feel free to just tell
> me what the solution should be - thats what I was hoping for in the
> first place anyway.
>
> Rupert
>
> On Tue, Aug 5, 2008 at 6:10 PM, Rupert Brooks <rupe.brooks at gmail.com> wrote:
>> Stephen,
>>
>> Yes, your example describes the problem perfectly.  There are no CMake
>> options that I'm aware of that would change it.  I think it happens in
>> every case.
>>
>> Your second comment brings up an important issue - aand a similar
>> problem has occurred.  The image derivatives on the oriented image
>> were previously not transformed by the image directions.  This lead to
>> problems and has been fixed, with a CMake switch.  However, the
>> SpatialObject and Image coordinate systems seem to be pretty much
>> separate code bases.   If derivatvies are taken relative to
>> SpatialObjects, then a similar problem *could* arise.  Perhaps it is
>> correct now, i havent checked.  Does anyone know?
>>
>> Still, i think this second point is a separate issue.  It seems the
>> design calls for the image IndexToPhysicalPoint transformation to
>> equate to the SpatialObject IndexToWorld transform - and thats not
>> causing problems.  Its just the unexpected existence of directions on
>> a itk::Image that does.
>>
>> Rupert
>>
>>
>> On Tue, Aug 5, 2008 at 4:28 PM, Stephen Aylward
>> <Stephen.Aylward at kitware.com> wrote:
>>> Hi Rupert,
>>>
>>> Am I understanding the problem correctly...
>>>
>>> ImageType::Pointer img = ImageType::New();
>>> img->SetDirections( someNonIdentityMatrix );
>>> img->TransformPhysicalPointToIndex(pnt, indx);
>>> std::cout << img->GetDirections() << std::endl;
>>>
>>> Will produce the exact same output (i.e., have the same directions) if
>>> ImageType is an itkImage or an itkOrientedImage, BUT the value of indx
>>> (which isn't printed) will potentially be very different.   That is,
>>> the images will have the same set of directions, but one will use it
>>> and one won't.
>>>
>>> We actually won't know what the code does unless we know what cmake
>>> options were set, right?
>>>
>>> Regretfully the concept of image direction was added after spatial
>>> objects were created.   ImageSpatialObjects (like many other spatial
>>> objects) have transforms which move from index space to object space
>>> and from object space to world space.   For imagespatialobjects, in
>>> the index space to object space transform, voxel spacing is
>>> considered.  For object space to world transform, the object (and its
>>> children-objects, e.g., extracted surfaces) are transformed to place
>>> them in the scene.
>>>
>>> So, the question becomes, should the directions in an image be applied
>>> to objects extracted from the image, or not?
>>>
>>> As specific examples, consider meshes or paths extracted from an image
>>> using one of the existing filters.   When you extract a mesh or a path
>>> from an image, in what space does it live: index space, object space,
>>> or world coordinates?   When considering this, it is particularly
>>> interesting to look at how  normals / gradients at node points are
>>> computed (e.g., how paths are steered as they are extracted from an
>>> image).  Worst case, if the image coordinates are transformed by the
>>> directions, but the gradients at those coordinates aren't specifically
>>> transformed by the directions, then those entities live in a hybrid
>>> space with some info (coordinates) in world space and some info
>>> (gradients) in object space.
>>>
>>> If we assume all is good, then we should apply the directions when
>>> going from index to object space, otherwise, the directions should be
>>> applied when going from object space to world space, so that the
>>> directions are applied to children objects as well.
>>>
>>> Stephen
>>>
>>> On Tue, Aug 5, 2008 at 3:31 PM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
>>>> More questions:
>>>>
>>>> Can ImageSpatialObject be rewritten to use the Transform methods of
>>>> Image and OrientedImage?
>>>>
>>>> Bill
>>>>
>>>> On Tue, Aug 5, 2008 at 3:26 PM, Stephen Aylward
>>>> <Stephen.Aylward at kitware.com> wrote:
>>>>> So,
>>>>>
>>>>> Should an itkImage always return the identity matrix for its directions?
>>>>>
>>>>> or
>>>>>
>>>>> Should we allow inconsistency between recorded information and actual
>>>>> operation (record, but do not use direction in itkImage)?
>>>>>
>>>>> or
>>>>>
>>>>> Should we do-away with the itkImage without orientation, and should
>>>>> all images be itkOrientedImages?
>>>>>
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Aug 5, 2008 at 3:19 PM, Rupert Brooks <rupe.brooks at gmail.com> wrote:
>>>>>> Hi Bill,
>>>>>>
>>>>>> Thats just the thing - the ImageSpatialObject does NOT use
>>>>>> TransformXtoY calls, because it has its own internal transformation
>>>>>> system.  It copies the information out of the image that it is given
>>>>>> and uses that as the spatial object IndexToWorld transform.  The bug
>>>>>> was caused because previously it did not copy the direction - this is
>>>>>> easily fixed.
>>>>>>
>>>>>> The tricky issue is that itk::Image always behaves as though its
>>>>>> direction is identity - but this is not enforced.  Its entirely
>>>>>> possible to put a non-identity direction in itk::Image.  This is not
>>>>>> used in the TransformXtoY calls of the image - but it is returned by
>>>>>> the GetDirection method.  So when i copy the direction information, if
>>>>>> there is some in the itk::Image, it breaks the test.
>>>>>>
>>>>>> As you can probably guess, i discovered that by accident.
>>>>>>
>>>>>> I agree that itk::Image should not behave differently inside and
>>>>>> outside a Spatial object.   As I see it, theres 4 options - I think
>>>>>> its a design decision which is why im bringing it up here.
>>>>>>
>>>>>> 1. Explicitly test for the itk::Image case inside itk::SpatialObject,
>>>>>> and ignore its directions
>>>>>>    This will run into problems with subclasses of itk::Image - and
>>>>>> not all subclasses can be treated the same way.  Which leads to #2
>>>>>>
>>>>>> 2. Explicitly test for the itk::OrientedImage case inside
>>>>>> itk::SpatialObject and only use the direction in that case
>>>>>>    This runs into the reverse problem, that if another subclass of
>>>>>> itk::Image with direction information is used, it will break when used
>>>>>> in the itk::ImageSpatialObject.    Perhaps I am biased against this
>>>>>> because i actually have such a class in my code.   No such class
>>>>>> currently exists in ITK, that i know of.
>>>>>>
>>>>>> 3. Consider this a conceptual bug in itk::Image, and force the
>>>>>> itk::Image GetDirection method to always return identity.
>>>>>>    I think this is the most correct answer, but i fear backward
>>>>>> compatibility consequences.
>>>>>>
>>>>>> 4. Ignore the issue, and assume/hope that users wont put direction
>>>>>> cosines in an itk::Image anyway.
>>>>>>    This is by far the easiest approach, but i don't like knowing a
>>>>>> potential bug exists and not fixing it.  These type of sins always
>>>>>> seem to come back to haunt me :-)
>>>>>>
>>>>>> Rupert
>>>>>>
>>>>>> On Tue, Aug 5, 2008 at 2:08 PM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
>>>>>>> Rupert,
>>>>>>>
>>>>>>> In general, itkImage directions are ignored (assumed identity).
>>>>>>> itkOrientedImage directions are used. I have not looked at the
>>>>>>> ImageSpatialObject yet. If it uses Transform{X}To{Y} type calls, then
>>>>>>> Image and OrientedImage should behave properly. I don't think itkImage
>>>>>>> should behave differently inside and outside of itkImageSpatialObject.
>>>>>>>
>>>>>>> Bill
>>>>>>>
>>>>>>> On Tue, Aug 5, 2008 at 1:58 PM, Rupert Brooks <rupe.brooks at gmail.com> wrote:
>>>>>>>> Hi Everyone,
>>>>>>>>
>>>>>>>> As Luis wisely suggested i waited until after the 3.8 release to work
>>>>>>>> on this  :-)  I still need some big picture advice thoug
>>>>>>>>> Please note that the first action to take, even before experimenting
>>>>>>>>> with a fix, is to add a tests that illustrates the failure. E.g. to
>>>>>>>>> add a test that exercise the condition you have identified as
>>>>>>>>> problematic.
>>>>>>>>
>>>>>>>> I've added the test (itkImageMaskSpatialObjectTest2), and you all
>>>>>>>> should notice it failing in your dashboards shortly :-)
>>>>>>>>
>>>>>>>> Theres more than one way to patch the problem - i'll refer back to my
>>>>>>>> previous email.  I think theres a conceptual bug, or at least
>>>>>>>> confusion in the itk::Image. I cant decide how i should make
>>>>>>>> ImageSpatialObject behave when handed an ITK image that has
>>>>>>>> non-identity directions.
>>>>>>>>
>>>>>>>>>> Briefly, the image spatial object takes the coordinate transform from
>>>>>>>>>> the image that it is given, and uses that as its IndexToWorld
>>>>>>>>>> transformation.  Previously, it was ignoring the direction.  The fix
>>>>>>>>>> was a matter of adding the necessary lines to copy the direction
>>>>>>>>>> information also.
>>>>>>>>>>
>>>>>>>>>> However, this fix raises a further issue, and I'd like advice:
>>>>>>>>>>
>>>>>>>>>> The itk::Image class may have a non-identity direction component, but
>>>>>>>>>> it always ignores it.  However, a naive implementation of my fix would
>>>>>>>>>> use that direction information, giving a different behavior than the
>>>>>>>>>> itk::Image.  I could write code to check if its an ITK image, and then
>>>>>>>>>> behave differently, but this seems inelegant at best.   Is this
>>>>>>>>>> actually another bug - should an itk::Image always have an identity
>>>>>>>>>> direction, to conform with its behavior?  Can i ignore the issue,
>>>>>>>>>> assuming the user will be bright enough to only put direction
>>>>>>>>>> information in an itk::Image for a very good reason.
>>>>>>>>
>>>>>>>> Does anyone have some insights into how the itkImageSpatialObject
>>>>>>>> should behave when handed an itk::Image which has non-identity
>>>>>>>> direction cosines?
>>>>>>>>
>>>>>>>> (the bug in question is this one
>>>>>>>> http://public.kitware.com/Bug/view.php?id=0006340)
>>>>>>>>
>>>>>>>> Cheers
>>>>>>>> Rupert B.
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> --------------------------------------------------------------
>>>>>>>> Rupert Brooks
>>>>>>>> McGill Centre for Intelligent Machines (www.cim.mcgill.ca)
>>>>>>>> Ph.D Student, Electrical and Computer Engineering
>>>>>>>> http://www.cyberus.ca/~rbrooks
>>>>>>>> _______________________________________________
>>>>>>>> Insight-developers mailing list
>>>>>>>> Insight-developers at itk.org
>>>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Insight-developers mailing list
>>>>>>> Insight-developers at itk.org
>>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> --------------------------------------------------------------
>>>>>> Rupert Brooks
>>>>>> McGill Centre for Intelligent Machines (www.cim.mcgill.ca)
>>>>>> Ph.D Student, Electrical and Computer Engineering
>>>>>> http://www.cyberus.ca/~rbrooks
>>>>>> _______________________________________________
>>>>>> Insight-developers mailing list
>>>>>> Insight-developers at itk.org
>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Stephen R. Aylward, Ph.D.
>>>>> Chief Medical Scientist
>>>>> Kitware, Inc. - Chapel Hill Office
>>>>> http://www.kitware.com
>>>>> (518) 371-3971 x300
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Stephen R. Aylward, Ph.D.
>>> Chief Medical Scientist
>>> Kitware, Inc. - Chapel Hill Office
>>> http://www.kitware.com
>>> (518) 371-3971 x300
>>>
>>
>>
>>
>> --
>> --------------------------------------------------------------
>> Rupert Brooks
>> McGill Centre for Intelligent Machines (www.cim.mcgill.ca)
>> Ph.D Student, Electrical and Computer Engineering
>> http://www.cyberus.ca/~rbrooks
>>
>
>
>
> --
> --------------------------------------------------------------
> Rupert Brooks
> McGill Centre for Intelligent Machines (www.cim.mcgill.ca)
> Ph.D Student, Electrical and Computer Engineering
> http://www.cyberus.ca/~rbrooks
>



-- 
--------------------------------------------------------------
Rupert Brooks
McGill Centre for Intelligent Machines (www.cim.mcgill.ca)
Ph.D Student, Electrical and Computer Engineering
http://www.cyberus.ca/~rbrooks