[Insight-developers] Dashboard woes

[Luis Ibanez]

On Thu, Jun 2, 2011 at 12:03 PM, Johnson, Hans J <hans-johnson at uiowa.edu> wrote:
>
> "The overall design of the toolkit is that
> classes should  do one thing and they
> should do it right."
> ========>   OK.  Do it "right" --> physical space
>

If physical space was part of the
original specification of the class, yes.


>
> While ITK itself remains a layer where:
> "one filter does one-thing"
> ========>   OK.  One filter does one-thing  --> add images together in
> physical space.  This same argument holds for the Binary/Ternary functor
> base classes.  Any filter that does not work with consistent physical
> space interpretation (NOTE:  I clearly state interpretation here, not
> necessarily that physical space has to be explicitly computed, just that
> it has to be checked for) should be instrumented to throw an exception or
> be over-riden by the end-user.
>
> We'll get right on this... :)
>
> Just kidding, but who decides what is right?


We do that together,
That's why we are having this conversation.


>  I see a lot of ITK
> documentation (powerpoint presentations, wiki-pages, the ITK users guide
> -- See excerpt from page 40 next, talks) that state that one MUST consider
> physical space whenever doing medical image processing.
>

We insist in that point in the context of Image Registration.

Note that there is no mention of image grid consistency
in the case of LevelSets, or Mathematical Morphology.

That said, those filters probably should check for image grid
consistency, given that their execution actually assumes that
the grids match.


> I'm do not find support stating that voxel space interpretations WITHOUT
> an implied physical space consistency has any benefit other than for
> performance.  ITK has tends to error on the side of producing correct
> results, even if it introduces a performance penalty, and requiring the
> end-user to explicitly request over-riding those constraints.
>

In this particular case, the concern is not performance.
Kent's patch is dealing properly with performance, by
only doing resampling when requested.

The concern here is long term maintenance,
proper design, and consistency across the toolkit.


> I believe very strongly that:
> """
> Even though ITK can be used to perform general image processing tasks, the
> primary purpose
> of the toolkit is the processing of medical image data. In that respect,
> additional information
> about the images is considered mandatory. In particular the information
> associated with the
> physical spacing between pixels and the position of the image in space
> with respect to some
> world coordinate system are extremely important.
>
> Image origin and spacing are fundamental to many applications.
> Registration, for example,
> is performed in physical coordinates. Improperly defined spacing and
> origins will result in
> inconsistent results in such processes. Medical images with no spatial
> information should not
> be used for medical diagnosis, image analysis, feature extraction,
> assisted radiation therapy or
> image guided surgery. In other words, medical images lacking spatial
> information are not only
> useless but also hazardous.
> """   ===> Page 40 of ITK Users Guide ===
>

All true,

Yet we read and write PNG, TIFF, BMPs...
that throw all this information away.

If we are going to clean up this behavior
on the entire toolkit, that is fine, but then
we should plan accordingly because this
is going to be more that one small patch.

We have to do the same in *every single*
filter...

We have to remove the file formats that
do not carry origin, spacing and orientation
(so we are left with DICOM, Meta and Nifti).

This would be a large scale undertaking,
and it has not been accounted for in the
many tasks that we have for ITKv4 and
for which we are running behind for the
Beta release.

>
> In summary:
> 1) "The right" way is to do the operations in a way that is consistent
> with physical space,

Not always.
An application can enforce that consistency.

> 2) When index space and physical space of all images are consistent, we
> should provide a highly optimized index-only computation (NOTE:  This
> still maintains consistency of the physical spaces)

Agree

> 3) The default behavior for when images are not in consistent spaces is to
> fail, but can be explicitly over-riden by an end-user who chooses to play
> with fire.
>

Back to Bill's point on design:
Why should the switch be made in the form of a "mode",
when it could be done in the form of a new "object".

A "mode" will be appropriate if one could expect that
an application may want to change the behavior of
this class at run time.

Do we envision an application turning on/off this
switch at run time ?


> Why should adding two images together be allowed to break such fundamental
> tenants of ITK?
>

It doesn't have to.

We can simply do

A) Current filters, that assume grid consistency
     should check for it, and throw exceptions when
     the grids do not match.

B) Specialized filters can be added to allow for
     operations between two images that are not
     in the same image grid.

Notice that resampling images on the fly is
"convenient" for those who know what they are
doing, by it is as dangerous as the previous
behavior for those who are not paying attention
to the image grids.

At least by implementing this behavior in
specialized classes (with proper names)
it is clearer what to expect from these filters.

So, classes in (B) could be called

     AddInPhysicalSpaceImageFilter

and their documentation must specify
that they will resample one of the images.

They should also allow to use different
Interpolators, not just the linear one.


     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 11:27:13 -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
>
> 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.
>> ________________________________
>>
>
>
>
> ________________________________
> 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.
> ________________________________
>