[Insight-developers] Global Code Reviews

[Luis Ibanez]

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
>> >> >>
>> >> >
>> >> >
>> >
>> >
>
>