[Insight-developers] ImageSpatialObject Bug0006340

[Stephen Aylward]

Thanks for all of the help and for bringing this to the surface again.

Feel free to add to the wish-list wiki at any time.   Hopefully you
got us going soon enough that it might actually get "resolved" in 4.0

Thanks,
Stephen

On Wed, Aug 6, 2008 at 4:03 PM, Rupert Brooks <rupe.brooks at gmail.com> wrote:
> 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
>



-- 
Stephen R. Aylward, Ph.D.
Chief Medical Scientist
Kitware, Inc. - Chapel Hill Office
http://www.kitware.com
(518) 371-3971 x300