[Insight-developers] Global Code Reviews

Tue Apr 26 18:01:39 EDT 2011

Hi Bill,

That's an offer that we can't reject.      :-)

We will be happy to include lunch,
and even a beer.

Let me send you a doodle link to find a time.

    Thanks

          Luis

--------------------------------------------------------------
On Tue, Apr 26, 2011 at 5:39 PM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
> Luis,
>
> I think in a 1 hour tcon we could do the assignments. Then use SLicer4,
> SNAP, WikiExamples to establish priorities.
>
> Or, on a rainy day (or non Golf day), I could stop by Kitware and we could
> spend a couple of hours and so an initial assigment (assuming you'll buy
> lunch).
>
> Bill
>
>
>
> On Tue, Apr 26, 2011 at 5:10 PM, Luis Ibanez <luis.ibanez at kitware.com>
> wrote:
>>
>> Hi Bill,
>>
>> That's a good idea,
>>
>> Unfortunately we have not made that assignment
>> of Review classes to modules.
>>
>> I would suggest that we identify first the few classes
>> from Review that have proved to be of great use to
>> applications (e.g. Slicer, SNAP,....), and move those.
>>
>> In a second pass we can do a full triage of the Review
>> directory.  It may turn to be easier to do this assignments
>> as we code review each one of the files.
>>
>>
>>    My 2 cents,
>>
>>
>>        Luis
>>
>>
>>
>> --------------------------------------------
>> On Tue, Apr 26, 2011 at 4:43 PM, Bill Lorensen <bill.lorensen at gmail.com>
>> wrote:
>> > Luis,
>> >
>> > You may have already done this, but I think a good first pass would be
>> > to
>> > assign every class in review to an itk module. Then, when it comes time
>> > to
>> > move or reject them, we'll know where there should or should have gone.
>> >
>> > Bill
>> >
>> > On Tue, Apr 26, 2011 at 3:49 PM, Luis Ibanez <luis.ibanez at kitware.com>
>> > wrote:
>> >>
>> >> Sorry,
>> >> My bad,
>> >> I should have described the process better.
>> >>
>> >>
>> >>    Luis
>> >>
>> >> -----------------------------------
>> >> On Tue, Apr 26, 2011 at 3:10 PM, Bill Lorensen
>> >> <bill.lorensen at gmail.com>
>> >> wrote:
>> >> > I agree 100%. I thought the original message implied B) then A). A)
>> >> > then
>> >> > B)
>> >> > sounds good.
>> >> >
>> >> > Bill
>> >> >
>> >> > On Tue, Apr 26, 2011 at 2:43 PM, Luis Ibanez
>> >> > <luis.ibanez at kitware.com>
>> >> > wrote:
>> >> >>
>> >> >> Hi Bill,
>> >> >>
>> >> >> Let's rephrase it as:
>> >> >>
>> >> >>
>> >> >> A)   "Move the Useful classes out of Review and into final Modules"
>> >> >>
>> >> >> B)  "Move the remaining Review directory to an External module"
>> >> >>
>> >> >>
>> >> >> Along the lines of (A), we will be reviewing, cleaning and bringing
>> >> >> up to ITK standards the following class:
>> >> >>
>> >> >>                 itkMemoryProbesCollectorBase
>> >> >>
>> >> >> This will remove the only dependency that the Examples have
>> >> >> on the Review directory.
>> >> >>
>> >> >>
>> >> >> The  subsequent question is:
>> >> >>
>> >> >>   What classes is Slicer needing from the ITK Review directory ?
>> >> >>
>> >> >> We will list those classes, code review them, bring them up to
>> >> >> ITK standards and put them in their final Module locations.
>> >> >>
>> >> >>
>> >> >> ---
>> >> >>
>> >> >> Review currently has 358 files.
>> >> >>
>> >> >> About the same that used to be in the old "BasicFilters"
>> >> >> directory.
>> >> >>
>> >> >> Over time we have been confusing:
>> >> >>
>> >> >> "My app depends on class A that happens to be in Review"
>> >> >>
>> >> >> with
>> >> >>
>> >> >> "My app depends on the Review directory...."
>> >> >>
>> >> >> (where the second englobes 358 files...)
>> >> >>
>> >> >>
>> >> >> Triaging the Review directory is one of the top priorities
>> >> >> in the general code review of the toolkit.
>> >> >>
>> >> >>
>> >> >>
>> >> >>      Luis
>> >> >>
>> >> >>
>> >> >> -----------------------
>> >> >> On Tue, Apr 26, 2011 at 1:45 PM, Bill Lorensen
>> >> >> <bill.lorensen at gmail.com>
>> >> >> wrote:
>> >> >> > I did not mean "clearing" literally.
>> >> >> >
>> >> >> > My concern is with the external project movement. I think we
>> >> >> > should
>> >> >> > review
>> >> >> > the "Review" classes, move/repair the ones that satisfy "some
>> >> >> > criteria".
>> >> >> > Once we have done this, then we can create an External Package.
>> >> >> >
>> >> >> > There are so many changes to our software process, I'm concerned
>> >> >> > that
>> >> >> > adding
>> >> >> > another step (working with an external process) will hurt our
>> >> >> > productivity.
>> >> >> > I ajm also concerned that there are only a handfull of people that
>> >> >> > understand the current, new process.
>> >> >> >
>> >> >> > Bill
>> >> >> >
>> >> >> > On Tue, Apr 26, 2011 at 1:40 PM, Bradley Lowekamp
>> >> >> > <blowekamp at mail.nih.gov>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> Hello Bill,
>> >> >> >> I am not sure that "clearing" out review it the correct way to
>> >> >> >> view
>> >> >> >> thing.
>> >> >> >> This phrase implies to me moving everything out of review and
>> >> >> >> into
>> >> >> >> ITK
>> >> >> >> proper.
>> >> >> >> There are many good filters that people have been using for quite
>> >> >> >> a
>> >> >> >> while.
>> >> >> >> These certainly should be moved. While there are others that have
>> >> >> >> not
>> >> >> >> used
>> >> >> >> and still need work. While others should replace filters in ITK
>> >> >> >> or
>> >> >> >> are
>> >> >> >> otherwise redundant.
>> >> >> >> Luis has certainly put together a good list of things to check,
>> >> >> >> and
>> >> >> >> it's a
>> >> >> >> good list regarding the effort.
>> >> >> >> Please only move filters that you can expect the community to be
>> >> >> >> able
>> >> >> >> to
>> >> >> >> trust the results of. (itkFFTComplexToComplexImageFilter has no
>> >> >> >> implementation, for non-FFTW and
>> >> >> >> segfaults, http://www.itk.org/Bug/view.php?id=11934)
>> >> >> >> Or perhaps we could start look into the filters where are
>> >> >> >> currently
>> >> >> >> in
>> >> >> >> ITK
>> >> >> >> and not up to the standards. (BinaryPrunningImageFilter,
>> >> >> >> BinaryThinningImageFilter)
>> >> >> >> Or perhaps we could start looking into the redundant filters in
>> >> >> >> ITK.
>> >> >> >> (ErodeObjectMorphologyImageFilter, and
>> >> >> >> DilateObjectMorphologyImageFilter no
>> >> >> >> one can tell me how this is different then setting for foreground
>> >> >> >> and
>> >> >> >> background values in the BinaryDilateImageFilter )
>> >> >> >>
>> >> >> >>
>> >> >> >> *reads Luis's e-mail*
>> >> >> >> On Apr 26, 2011, at 1:12 PM, Luis Ibanez wrote:
>> >> >> >>
>> >> >> >> As we discussed during the Boston meeting, a global
>> >> >> >> code review effort will take place before the Beta release.
>> >> >> >>
>> >> >> >> The code assignments to contractors that Jim created,
>> >> >> >> have been posted at:
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> https://spreadsheets.google.com/a/kitware.com/spreadsheet/ccc?key=t8spmfVf1nHiKKkNnVunkUQ&authkey=CM-C2vcL#gid=0
>> >> >> >>
>> >> >> >>
>> >> >> >> We are organizing the procedure for the code reviews in:
>> >> >> >> http://www.itk.org/Wiki/ITK_Release_4/Global_Code_Review
>> >> >> >>
>> >> >> >>
>> >> >> >> One of the first action items will be to move the Review
>> >> >> >> directory out of the repository and make it available as
>> >> >> >> an External module.
>> >> >> >>
>> >> >> >>
>> >> >> >> Then we will identify the classes from Review that are
>> >> >> >> commonly used (e.g. in applications like Slicer), and will
>> >> >> >> put those classes in the top priorities of code review to
>> >> >> >> move them into their final destination inside of the toolkit.
>> >> >> >>
>> >> >> >>
>> >> >> >>   Please share your thoughts on this process,
>> >> >> >>
>> >> >> >>
>> >> >> >> This process make sense to me.
>> >> >> >>
>> >> >> >>
>> >> >> >>       Thanks
>> >> >> >>
>> >> >> >>
>> >> >> >>            Luis
>> >> >> >> _______________________________________________
>> >> >> >> 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
>> >> >> >>
>> >> >> >> ========================================================
>> >> >> >>
>> >> >> >> Bradley Lowekamp
>> >> >> >>
>> >> >> >> Lockheed Martin Contractor for
>> >> >> >>
>> >> >> >> Office of High Performance Computing and Communications
>> >> >> >>
>> >> >> >> National Library of Medicine
>> >> >> >>
>> >> >> >> blowekamp at mail.nih.gov
>> >> >> >>
>> >> >> >>
>> >> >> >> _______________________________________________
>> >> >> >> 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
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>