[Insight-developers] PathToImageFilter issues

[Zachary Pincus]

My refactored PathToImageFilter and related files, a test of the same  
(that I made in the process of debugging the refactored code), and a  
brief document describing the changes are available here:
http://insightsoftwareconsortium.org/InsightJournal/view_reviews.php? 
back=index.php&pubid=74

There are definitely several issues in this code that would benefit  
from some review before the changes make their way into ITK. (These  
are described in the document.)

Note that these changes also encompass a the addition of a static  
constant PathDimension to itkPath.h -- that's why that file is in the  
tarball.

Zach

On Feb 13, 2006, at 3:38 PM, Miller, James V (GE, Research) wrote:

> Zachary,
>
> I wasn't really suggesting the new code (with all your refactoring)
> would be something that the IJ would build.  I was just suggesting
> it as a place to the code could be posted for others to review.
> I imagine if there is no CMakeLists.txt file, then the IJ
> will not attempt to build the code. The benefit of the IJ is that
> you could also post a document describing the new code and the
> refactorings.
>
> I'd prefer the code posted as complete files rather than patches.
> My ability to browse or apply a patch on my Windows system has
> never been resurrected from when I replaced my laptop last year.
>
> An alternative is to post these to the bug tracker.  I just thought
> the IJ with its document management system would be a better place.
>
> Another alternative is create a (short lived) branch on which you
> put the code.
>
> Jim
>
>
> -----Original Message-----
> From: Zachary Pincus [mailto:zpincus at stanford.edu]
> Sent: Monday, February 13, 2006 1:55 PM
> To: Miller, James V (GE, Research)
> Cc: insight-developers at itk.org; Lorensen, William E (GE, Research)
> Subject: Re: [Insight-developers] PathToImageFilter issues
>
>
> Hi folks -
>
> It might be good to get these fixes into the next revision of ITK.
> Given that the fix necessarily involves deprecating some
> functionality and moving it to another class, it might be good to get
> people using the proper class sooner rather than later. More
> importantly, the current PathToImageFilter "base class" is impossible
> to properly subclass from without these fixes. However, you guys
> probably have enough to do already, so I'll defer to you on this.
>
> In general, I'm not totally sure the IJ is the best forum for
> reviewing maintenance fixes to existing ITK code, especially with
> regard to hewing to ITK conventions. (Note that the
> PathToImageFilter, cause of these issues, is part of the second-
> highest-ranked submission to the IJ.)
>
> For one, the IJ build environment doesn't have (as far as I can tell)
> a CVS version of ITK, so you can't submit code and be sure that it
> will work with the latest ITK. Moreover, submitted code can't use
> things only in the CVS. Both of these are obviously a must for bug-
> fix issues. (Already with my previous IJ submission, there are some
> things that will have to be changed when/if it is incorporated into
> ITK; e.g. the CVS has the more expressive
> "InvalidRequestedRegionError" and 2.4.1 does not.)
>
> Moreover, there's no mechanism in IJ to submit patches to code
> already in the CVS. Thus, to discuss these changes, I'd have to
> submit new Path and PathToImageFilter classes to the IJ, and hope the
> include paths on the build machine get set correctly so the new
> classes are found instead of the current versions of the same. Then
> to integrate the changes into ITK, someone would have to manually
> diff them against the current version and roll the changes in, and so
> forth.
>
> Perhaps there could be a lighter-weight mechanism for reviewing bug-
> fix changes, etc? I agree that peer-review of code (bug-fixes and new
> submissions) is important, but while using the IJ to "raise the bar"
> for new submissions (in terms of quality and usefulness) is clearly a
> good idea, I'm not sure that raising the bar for people trying to fix
> existing problems is really the best approach. Likewise, I'm not sure
> that my constant spamming of the dev list proposing bug fixes is
> optimal either!
>
> Zach
>
>
>
> On Feb 13, 2006, at 11:49 AM, Miller, James V (GE, Research) wrote:
>
>> Zachary,
>>
>> I think posting the classes on the Insight Journal would allow a
>> number of people
>> to evaluate the changes.  Already I would be tempted to rename
>> TracePathOnImageFilter
>> to TracePathImageFilter.
>>
>> We are about to lock down the repository (if it wasn't already done
>> last night)
>> in preparation of the next release.  Your changes may have to wait
>> until the
>> repository re-opens after the release is tagged.
>>
>> Jim
>>
>> -----Original Message-----
>> From: insight-developers-bounces+millerjv=crd.ge.com at itk.org
>> [mailto:insight-developers-bounces+millerjv=crd.ge.com at itk.org]On
>> Behalf
>> Of Zachary Pincus
>> Sent: Monday, February 13, 2006 11:33 AM
>> To: insight-developers at itk.org
>> Cc: Lorensen, William E (GE, Research)
>> Subject: Re: [Insight-developers] PathToImageFilter issues
>>
>>
>> Hi folks,
>>
>> I've re-worked (in what I think is a 100% backward-compatible way)
>> the PathToImageFilter class to deal with the issues that I brought up
>> last week. Should I go ahead and commit the new class (and a new
>> subclass, TracePathOnImageFilter, which has the functionality that
>> has been deprecated from PathToImageFilter)?
>>
>> Perhaps it would be better if I send the classes to someone else for
>> review to make sure I haven't broken compatibility, abused ITK
>> convention, etc? (To avoid a re-play of the issue of having an
>> incorrectly-defined base class in the future.) Any volunteers?
>>
>> Zach
>> _______________________________________________
>> 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