[Insight-developers] Dashboard woes
Luis Ibanez
luis.ibanez at kitware.com
Thu Jun 2 11:27:13 EDT 2011
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.
> ________________________________
>
More information about the Insight-developers
mailing list