[Insight-developers] Dashboard woes
Johnson, Hans J
hans-johnson at uiowa.edu
Thu Jun 2 10:55:14 EDT 2011
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:
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.
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 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.
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.
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.
________________________________
More information about the Insight-developers
mailing list