[Insight-developers] ImageSpatialObject Bug0006340

Rupert Brooks rupe.brooks at gmail.com
Wed Aug 6 11:48:35 EDT 2008 

Yes it seems like this bug has dredged up deeper issues.  I think the
itk::Image behavior issue should be put off to another discussion.

To attempt to close out this bug heres what i propose.
 - I'll fix the errors in my submission that Bill pointed out *blush*
 - I'll add my patch
 - I will include a specific test for the itk::Image case in the
ImageSpatialObject, and force it to ignore directions in that case.
This is a bit kludgy, but otherwise the behavior of ImageSpatialObject
would suddenly change, and i think this would break existing code.

Unless there is vocal protest, I should get that in by tomorrow.

Cheers,
Rupert B.

On Wed, Aug 6, 2008 at 10:05 AM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
> I don't think we have agreement that the next release will be 4.0. I
> thought we might consider making it 3.10.
>
> Also, I don't think we have agreement that we will be breaking
> backward compatibility in several places.
> Let's move this discussion to another e-mail thread.
>
> Bill
>
> On Wed, Aug 6, 2008 at 8:56 AM, Stephen Aylward
> <Stephen.Aylward at kitware.com> wrote:
>> Set/GetDirection functions should not be in itkImage.
>> * For an itkImage, direction info should be stored in the meta data
>> dictionary - like modality type, etc.
>> * For an itkOrientedImage, those functions should exist.
>>
>> However, there were reasons for this design.   The real problem is
>> that the oriented image implementation was never fully carried
>> through.   Two/three years later we're still paying the price.
>>
>> If you want to patch this hole, probably the best solution is the following:
>>
>> 1) when you read into an image, the directions used by the get/set
>> directions functions are set to identity and the real direction
>> information is stored in the meta data dictionary.
>>
>> 2) On writing an image, the data in the meta data dictionary is
>> written as the directions in the file.   That way, we don't loose
>> direction information by reading/writing a file into an image.
>>
>> The can be done fairly high in the IO framework so that it affects all
>> readers/writers - instead of changing the readers/writers
>> individually.
>>
>> Ultimately, we should make all images in ITK oriented.   This would
>> break backward compatibility for people who have orientation
>> information in their files and have written custom filters that don't
>> use orientation (or use it via get/setdirection, or ...).
>> Regardless, we shouldn't try to keep backward compatibility in such
>> cases or we'll end up buried in confusing cmake variables.   We should
>> plan on this for release 4.0 when backward compatibility will be
>> broken for various other parts of the toolkit so that it can be
>> adapted to the evolving field.
>>
>> Luis - are we going to start a branch to begin work on the 4.0 release?
>>
>> Stephen
>>
>> 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
>>>
>>
>>
>>
>> --
>> 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

More information about the Insight-developers mailing list