[Insight-developers] Verifying physical regions

[Williams, Norman K]

It isn't precisely true that there's no checking, because I ran afould of
it working in the FunctorImageFilters.

Look at ProcessObject::PropogateRequestedRegion().  At the bottom,
there's a loop, that calls DataObject::PropogateRequestedRegion() on all
inputs.

DataObject::PropogateRequestedRegion() calls
DataObject::VerifyRequestedRegion(),
which in the case of Images, verifies that the requested region is a
proper subspace of the Image's total region.

It does so in Index Space, not Physical space.

Verifying the requested region in physical space would require some
significant changes to the pipeline mechanism, because at this point,
there's no communication of information about physical space propogated
backwards and forwards in the pipeline.

At the filter level, you could certainly do what my FunctorImagePatch did
-- if the inputs are Images, get their orientation,location,and spacing
and see if they match.




On 6/2/11 11:23 AM, "Bradley Lowekamp" <blowekamp at mail.nih.gov> wrote:


>Hans,
>
>It make since to me to require that images line up in physical space
>before performing index base operations. In fact I would be in favor of
>moving this check way up in the pipeline for all filters, and when this
>behavior is not desired having to overload a method. I have suggested
>adding a new method in the pipeline, called something like,
>VerifyAllInputs(), which is called before the UpdateOutputInformation()
>methods are called. This method could verify that the required inputs are
>set and that they match is size, and physical location. Currently, many
>filters don't check that any of this information matches.
>
>Luis,
>
>On Jun 2, 2011, at 12:05 PM, Luis Ibanez wrote:
>
>> On Thu, Jun 2, 2011 at 12:00 PM, Bradley Lowekamp
>> <blowekamp at mail.nih.gov> wrote:
>>> Hello,
>>>
>>> For some reason my e-mail sometimes seem to be very slow to get to the
>>>developers list.
>>>
>>> I have suggested writing an ImageAdaptor  which does the interpolating
>>>as opposed to a new filter hierarchy. This should be more flexible, fix
>>>more filters for the physical space alignment, and be more maintainable
>>>in the long run, along with likely being less code and new classes too.
>>>
>>
>> Brad,
>>
>> Are you referring to ImageAdaptors as we already have them
>> implemented ? or are you using that term loosely ?
>
>I would think that only minor changes to the ImageAdaptor is needed to
>enable it to perform linear interpolation at each sample.
>
>>
>> ImageAdaptors, as currently implemented in ITK are only
>> suitable for Pixel-wise operations, and are unaware of
>> many of the transactions that filters have to deal with.
>
>The ImageAdaptor is a data object not a process object, so I am not sure
>about what you are referring to.
>
>I was thinking of being able to define a filter as:
>
>itk::AddIImageFilter< InputImageType,
>ColocatedImageAdaptor<InputImageType>, OutputImageType>.
>
>Where the adaptor object would need some information from the first input
>object, to ensure they are on the same grid.
>
>Brad
>
>
>>
>>
>>      Luis
>>
>>
>>>
>>> Brad
>>>
>>> On Jun 2, 2011, at 11:43 AM, Williams, Norman K wrote:
>>>
>>>> How about this -- once Luis's patch backing out my changes are in
>>>>place, I
>>>> will do a patch that creates a module for physical space
>>>>binary/ternary
>>>> functor classes.
>>>>
>>>>
>>>> Since the changes to the existing class are minimal and
>>>>straightforward, I
>>>> can derive the new classes from the existing functor classes.  There
>>>>would
>>>> be no 'UsePhysicalSpace' attribute.
>>>>
>>>> I would implement Multiplication/Division/Addition/Subtraction for 2
>>>> images and add for 3 images.
>>>>
>>>> I would use Interpolators rather than ResampleImageFilter unless
>>>>someone
>>>> can explain why using a ResampleImageFilter would actually be better.
>>>>
>>>> Actually there's one advantage of ResampleImageFilter -- when the
>>>>first
>>>> image parameter has a larger size than the second (and-or third) the
>>>> interpolator reports that the physical point of edge pixels on the
>>>>first
>>>> image are outside the image volume of the second.  This wouldn't
>>>>happen
>>>> with ResampleImagefilter, and I'm not sure how to make the
>>>>Interpolators
>>>> behave the same way.
>>>>
>>>>
>>>> On 6/2/11 10:27 AM, "Luis Ibanez" <luis.ibanez at kitware.com> wrote:
>>>>
>>>>> On Thu, Jun 2, 2011 at 10:55 AM, Johnson, Hans J
>>>>><hans-johnson at uiowa.edu>
>>>>> wrote:
>>>>>> OK.
>>>>>>
>>>>>> --I'll take responsibility for the mistakes make in this approach.
>>>>>>
>>>>>> There are two separate issues here, let's not let the first unduly
>>>>>> influence our decisions on the second:
>>>>>>
>>>>>
>>>>> Agree,
>>>>> there are two separate issues here.
>>>>>
>>>>>
>>>>>> 1) The redness of the dashboard.  This is somewhat tangential to
>>>>>>the the
>>>>>> algorithmic concerns.  It illustrates that more experience and
>>>>>>rigor is
>>>>>> needed to avoid introducing inter-module dependancies.  It was my
>>>>>> over-eagerness to get this in place that is the cause of these
>>>>>>problems.
>>>>>>
>>>>>
>>>>>
>>>>> It turns out that our software process has a window
>>>>> of opportunity for compilation errors to go unnoticed.
>>>>>
>>>>> a) We currently review every Gerrit patch independently
>>>>>
>>>>> b) In this case, we are seeing the effect of two
>>>>>  combined patches leading to a red dashboard.
>>>>>
>>>>> c) The second patch exposes an incomplete
>>>>>   aspect of the first patch.
>>>>>
>>>>> d) One of the culprits here is the absence
>>>>>  of unit testing in:
>>>>>
>>>>>      ITK/Modules/Filtering/ImageFilterBase/test
>>>>>
>>>>>   for the itkBinaryFunctorImageFilter class,
>>>>>   there is no unit test.
>>>>>
>>>>>   Should we have had a unit test here, the
>>>>>   dependency problem will have been spotted
>>>>>   in Kent's patch.
>>>>>
>>>>>   I'm now adding a unit test for this class.
>>>>>
>>>>>
>>>>> This reinforces the original point about:
>>>>>
>>>>> What is it that we should be doing now in ITKv4 ?
>>>>>
>>>>> - Increasing code coverage
>>>>> - Performing code reviews of the 2,300 files in the toolkit
>>>>> - Cleaning up the Review directory
>>>>> - Doing refactoring
>>>>> - Fixing bugs
>>>>>
>>>>>
>>>>>> 2) What the meaning of a "BinaryFunctorImageFilter" is for.
>>>>>>Perhaps it
>>>>>> should be "BinaryFunctorIndexSpaceImageFilter" and a new
>>>>>> "BinaryFunctorPhysicalSpaceImageFilter" needs to be created.  I
>>>>>>thought
>>>>>> (and still think) that it is possible for these to co-exists in one
>>>>>> filter, in a backwards compatible way, thus providing performance
>>>>>> advantages in the common case where index & physical spaces
>>>>>>coincide for
>>>>>> all images.
>>>>>>
>>>>>
>>>>> I agree that a new specialized class
>>>>> is what is required here.
>>>>>
>>>>> The overall design of the toolkit is that
>>>>> classes should  do one thing and they
>>>>> should do it right.
>>>>>
>>>>> Now that we have SimpleITK, the simplified
>>>>> watered down presentations of ITK features
>>>>> should be implemented at the level of that layer.
>>>>>
>>>>> While ITK itself remains a layer where:
>>>>>
>>>>>            "one filter does one-thing"
>>>>>
>>>>>
>>>>>> I know we've had this conversation (I think it was at the ITKv4
>>>>>>kickoff
>>>>>> meeting) regarding the add/subtract/multiply/divide image filters
>>>>>>and
>>>>>> what
>>>>>> the "correct" behavior should be.  There was clearly a need to
>>>>>>support
>>>>>> both the index space and the physical space computation methods.
>>>>>>Let's
>>>>>> focus on how to supply both feature sets rather than if we should
>>>>>> provide
>>>>>> both.  Should we have two separate class hierarchies?  I am
>>>>>>perfectly
>>>>>> happy if the answer is yes.  We'll back out the changes and
>>>>>>implement a
>>>>>> parallel hierarchal tree that supports physical space.
>>>>>>
>>>>>
>>>>> This sounds reasonable to me.
>>>>>
>>>>> In this way, only the applications that need such
>>>>> functionality will have to pay for the code overhead
>>>>> of the feature.
>>>>>
>>>>>
>>>>>> Ultimately, all I want is a way to add/subtract/multiply/divide
>>>>>>images
>>>>>> in
>>>>>> physical space in a way that is consistent and compatible with these
>>>>>> same
>>>>>> operations in index space.  The current approach seemed like a
>>>>>>logical
>>>>>> way
>>>>>> forward that minimized highly redundant and code duplication, and
>>>>>>also
>>>>>> would highlight other places in the code where one should consider
>>>>>> carefully what the correct physical space behavior should be.
>>>>>>
>>>>>> I can always go back to using my private physical space filters that
>>>>>> we've
>>>>>> used for years, but that seems selfish and wrong.
>>>>>>
>>>>>
>>>>> Agree,
>>>>> That's not the spirit of open source communities.
>>>>>
>>>>>
>>>>>   Luis
>>>>>
>>>>>
>>>>> -------------------------
>>>>>> Hans
>>>>>> --
>>>>>> Hans J. Johnson, Ph.D.
>>>>>> hans-johnson at uiowa.edu
>>>>>> Assistant Professor of Psychiatry
>>>>>> University of Iowa Carver College of Medicine
>>>>>> W278 GH, 200 Hawkins Drive
>>>>>>
>>>>>> Iowa City, Iowa 52242
>>>>>> Phone:  319-353-8587
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Luis Ibanez <luis.ibanez at kitware.com>
>>>>>> Date: Thu, 2 Jun 2011 10:17:15 -0400
>>>>>> To: Hans Johnson <hans-johnson at uiowa.edu>
>>>>>> Cc: Bill Lorensen <bill.lorensen at gmail.com>, ITK
>>>>>> <insight-developers at itk.org>
>>>>>> Subject: Re: [Insight-developers] Dashboard woes
>>>>>>
>>>>>> Hi Hans,
>>>>>>
>>>>>> The proper fix for this issue could have been to check for
>>>>>>consistency
>>>>>> on the grid parameters of the two input images in the BinaryFunctor
>>>>>> filter, and to throw exceptions when the grids are not consistent.
>>>>>>
>>>>>> The check should include:
>>>>>>
>>>>>>      1) Origin
>>>>>>      2) Spacing
>>>>>>      3) Orientation
>>>>>>      4) Number of pixels along every dimension
>>>>>>
>>>>>> and given that most of these are "float" values, a tolerance
>>>>>> must be involved in the comparison.
>>>>>>
>>>>>> The current introduction of the resampling functionalities in
>>>>>> the BinaryFunctorImageFilter is overloading what this filter
>>>>>> is supposed to do, and yet not fully solving the problem.
>>>>>>
>>>>>> All other filters in ITK that take multiple input are not being
>>>>>> fixed to match this new behavior: LevelSets, MorphoMath ?
>>>>>> ...even the TernaryFunctorImageFilter...
>>>>>>
>>>>>> The responsibility of the filter should go just up to checking
>>>>>> for consistency. Introducing the option of performing the
>>>>>> resampling into the filter itself converts this filter into a
>>>>>> small application.
>>>>>>
>>>>>> The change also has many arbitrary decisions.
>>>>>>
>>>>>> For example, it uses a LinerInterpolator
>>>>>> which is not the right option for any of the image filters
>>>>>> derived from BinaryImageFunctor that deal with binary
>>>>>> images or images of labels.
>>>>>>
>>>>>> The changes were implemented with the narrow scenario
>>>>>> of Addition and multiplication in mind, and disregard the
>>>>>> fact that the BinaryFunctorImageFilter is the parent of
>>>>>> many other filters in the toolkit.
>>>>>>
>>>>>> This is not just a "Math" filter.
>>>>>>
>>>>>>
>>>>>>    Luis
>>>>>>
>>>>>>
>>>>>> ---------------------------------
>>>>>> On Thu, Jun 2, 2011 at 9:56 AM, Johnson, Hans J
>>>>>><hans-johnson at uiowa.edu>
>>>>>> wrote:
>>>>>>> I dis-agree on the feature creep.  This is a longstanding
>>>>>>>deficiency in
>>>>>>> ITK that arose from an initial avoidance of oriented physical image
>>>>>>> space.
>>>>>>> Now is our chance to fix it!
>>>>>>>
>>>>>>> One of my favorite ITK powerpoint training slides is Bart Simpson
>>>>>>> writing
>>>>>>> "I will not register images in index space" over and over and over
>>>>>>>on a
>>>>>>> chalk board as punishment.  It is my opinion, (and hopefully the
>>>>>>> opinion
>>>>>>> of every student that I've ever taught) that image operations on
>>>>>>> medical
>>>>>>> images only make sense in physical space.  For those processing
>>>>>>>steps
>>>>>>> that
>>>>>>> work only on the voxel lattice, there is still an implied physical
>>>>>>> space
>>>>>>> that must be considered.  There are times when the developer can
>>>>>>>VERIFY
>>>>>>> THAT ALL THE PHYSICAL SPACES AND INDEX SPACES ARE CONSISTENT for
>>>>>>>both
>>>>>>> operand images, and then optimize the computation by using only the
>>>>>>> voxel
>>>>>>> lattice, thus skipping the implied sameness of the physical space.
>>>>>>>
>>>>>>> After all the effort put into making most of the ITK toolkit work
>>>>>>> properly
>>>>>>> in physical space, one still could not add two images together
>>>>>>>unless
>>>>>>> their voxel lattice was the same.  Even more disturbing, one could
>>>>>>>add
>>>>>>> two
>>>>>>> images together that had the same voxel lattice, but have
>>>>>>>completely
>>>>>>> different physical space representations, and no warning/error was
>>>>>>> thrown
>>>>>>> (I know this because it has bitten me several times,  I've begun to
>>>>>>> trust
>>>>>>> ITK to compute properly in physical space, or at least fail with
>>>>>>> rigor).
>>>>>>>
>>>>>>> It was naive to think that folding it into the current "Math"
>>>>>>>filters
>>>>>>> was
>>>>>>> the correct place to put this.  In my programs (and Slicers, and
>>>>>>> hopefully
>>>>>>> other medical imaging applications) there is a need to compute in
>>>>>>> physical
>>>>>>> space.
>>>>>>>
>>>>>>> ***
>>>>>>> * At a minimum, the filters that only work on voxel lattices
>>>>>>>should at
>>>>>>> least check that the physical space representations are
>>>>>>>compatible, and
>>>>>>> provide a warning if they are not.  There are too many 256^3 voxel
>>>>>>> medical
>>>>>>> images that do not have the same physical space representations.
>>>>>>> * A parallel set of operators is needed that can work on images in
>>>>>>> physical space, and if they have consistent physical space
>>>>>>> representations, it should fall trough to the highly optimized
>>>>>>>index
>>>>>>> only
>>>>>>> based computation.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hans
>>>>>>> --
>>>>>>> Hans J. Johnson, Ph.D.
>>>>>>> hans-johnson at uiowa.edu
>>>>>>> Assistant Professor of Psychiatry
>>>>>>> University of Iowa Carver College of Medicine
>>>>>>> W278 GH, 200 Hawkins Drive
>>>>>>>
>>>>>>> Iowa City, Iowa 52242
>>>>>>> Phone:  319-353-8587
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Luis Ibanez <luis.ibanez at kitware.com>
>>>>>>> Date: Thu, 2 Jun 2011 08:57:29 -0400
>>>>>>> To: Bill Lorensen <bill.lorensen at gmail.com>
>>>>>>> Cc: ITK <insight-developers at itk.org>
>>>>>>> Subject: Re: [Insight-developers] Dashboard woes
>>>>>>>
>>>>>>> Bill,
>>>>>>>
>>>>>>> This is the consequence of an incomplete analysis
>>>>>>> of dependencies when making changes.
>>>>>>>
>>>>>>> 1) The Changes in BinaryFunctorImageFilter
>>>>>>>  introduced the use of the LinearInterpolator
>>>>>>>  in a filter that up to now was quite self-sufficient.
>>>>>>>
>>>>>>> 2) BinaryFunctorImageFilter in in Core/Common
>>>>>>>
>>>>>>> 3) ITK-Common DEPENDS ITK-VNLInstantiation ITK-KWSys
>>>>>>>   (only)
>>>>>>>
>>>>>>> 4) With the changes in BinaryFunctor, now Common
>>>>>>>   depends (in practice) on the ITK-ImageFunctions module.
>>>>>>>   because that's where the Interpolators are. But, the
>>>>>>>   module dependency was not made explicit because...
>>>>>>>
>>>>>>> 5)  ITK-ImageFunction already depends on ITK-Common
>>>>>>>
>>>>>>> 6)  Ergo sum: Circular dependency
>>>>>>>
>>>>>>> 7) Why did this go unnoticed before ?
>>>>>>>
>>>>>>>   Because the test of the BinaryFunctor was made to
>>>>>>>   depend on the ITK-ImageFunction module, giving
>>>>>>>   the false impression that the problem was fixed.
>>>>>>>
>>>>>>> 8)  The recent relocation of the ImageCompose - related
>>>>>>>    filters exposed the missing dependency between
>>>>>>>    the BinaryFunctorImageFitler and the Interpolators.
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> I would have to agree with Brad L. on that this change
>>>>>>> in BinaryFunctorImageFilter is feature creep, and that
>>>>>>> we should reconsider the usefulness of that change.
>>>>>>>
>>>>>>>
>>>>>>>    Luis
>>>>>>>
>>>>>>>
>>>>>>> --------------------
>>>>>>> On Wed, Jun 1, 2011 at 11:55 PM, Bill Lorensen
>>>>>>> <bill.lorensen at gmail.com>
>>>>>>> wrote:
>>>>>>>> Folks,
>>>>>>>>
>>>>>>>> I suggest we hold off on any new merges except for those that will
>>>>>>>> bring the Dashboard back to normalcy. There is no sense in
>>>>>>>>checking in
>>>>>>>> code when there are so many compile errors on so many platforms.
>>>>>>>>
>>>>>>>> Bill
>>>>>>>> _______________________________________________
>>>>>>>> Powered by www.kitware.com
>>>>>>>>
>>>>>>>> Visit other Kitware open-source projects at
>>>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>>>
>>>>>>>> Kitware offers ITK Training Courses, for more information visit:
>>>>>>>> http://kitware.com/products/protraining.html
>>>>>>>>
>>>>>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>>>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>>>>>
>>>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Powered by www.kitware.com
>>>>>>>
>>>>>>> Visit other Kitware open-source projects at
>>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>>
>>>>>>> Kitware offers ITK Training Courses, for more information visit:
>>>>>>> http://kitware.com/products/protraining.html
>>>>>>>
>>>>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>>>>
>>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ________________________________
>>>>>>> Notice: This UI Health Care e-mail (including attachments) is
>>>>>>>covered
>>>>>>> by
>>>>>>> the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is
>>>>>>> confidential and may be legally privileged.  If you are not the
>>>>>>>intended
>>>>>>> recipient, you are hereby notified that any retention,
>>>>>>>dissemination,
>>>>>>> distribution, or copying of this communication is strictly
>>>>>>>prohibited.
>>>>>>> Please reply to the sender that you have received the message in
>>>>>>>error,
>>>>>>> then delete it.  Thank you.
>>>>>>> ________________________________
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ________________________________
>>>>>> Notice: This UI Health Care e-mail (including attachments) is
>>>>>>covered
>>>>>> by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521,
>>>>>>is
>>>>>> confidential and may be legally privileged.  If you are not the
>>>>>>intended
>>>>>> recipient, you are hereby notified that any retention,
>>>>>>dissemination,
>>>>>> distribution, or copying of this communication is strictly
>>>>>>prohibited.
>>>>>> Please reply to the sender that you have received the message in
>>>>>>error,
>>>>>> then delete it.  Thank you.
>>>>>> ________________________________
>>>>>>
>>>>> _______________________________________________
>>>>> Powered by www.kitware.com
>>>>>
>>>>> Visit other Kitware open-source projects at
>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>
>>>>> Kitware offers ITK Training Courses, for more information visit:
>>>>> http://kitware.com/products/protraining.html
>>>>>
>>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>>
>>>>> Follow this link to subscribe/unsubscribe:
>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>
>>>>
>>>>
>>>> ________________________________
>>>> Notice: This UI Health Care e-mail (including attachments) is covered
>>>>by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is
>>>>confidential and may be legally privileged.  If you are not the
>>>>intended recipient, you are hereby notified that any retention,
>>>>dissemination, distribution, or copying of this communication is
>>>>strictly prohibited.  Please reply to the sender that you have
>>>>received the message in error, then delete it.  Thank you.
>>>> ________________________________
>>>> _______________________________________________
>>>> Powered by www.kitware.com
>>>>
>>>> Visit other Kitware open-source projects at
>>>> http://www.kitware.com/opensource/opensource.html
>>>>
>>>> Kitware offers ITK Training Courses, for more information visit:
>>>> http://kitware.com/products/protraining.html
>>>>
>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>
>>>> Follow this link to subscribe/unsubscribe:
>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>
>>>
>



________________________________
Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged.  If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited.  Please reply to the sender that you have received the message in error, then delete it.  Thank you.
________________________________