[Insight-developers] What to do about statistics...

[Luis Ibanez]

Hi Jim,

Thanks for giving good example on the code review tasks !

On Wed, May 25, 2011 at 4:46 PM, Jim Miller <millerjv at ge.com> wrote:
> So I am trying to be a good little soldier and do my code reviews for ITKv4.
> (This could serve as a gentle reminder to all the contractors that we all
> have a stack of files to review and report.)  I decide its time to dive into
> the statistics library. Grab the first file that looks interesting,
> itkDecisionRuleBase. Fix up the documentation, add a missing overloaded
> method, remove an unnecessary PrintSelf implementation.  I am flying. Start
> working on the subclasses to itkDecisionRuleBase. Got a little sidetracked
> by a compiler error for a subclass I hadn't gotten to. Then I realize there
> is a class called itkDecisionRule. Look through that. Not a subclass it
> itkDecisionRuleBase. Odd. It doesn't overload the Evaluate() method. Odd.
> All its subclasses have the same name as those that derive from
> itkDecisionRuleBase but with a number "2" at the end of the name. Again odd.


Odd indeed.
This is a historic artifact of how the Statistics were refactored in the
review directory, and how the case of a refactored class was managed.

The very completed ITK Statistics Migration Guide explains it:
http://www.itk.org/Wiki/Proposals:Refactoring_Statistics_Framework_2007_Migration_Users_Guide#Decision_rules


> Realized that DecisionRule is in the statistics namespace but
> DecisionRuleBase is not. Odd. Start doing "git log" on files. Discover I
> need to use "git log --follow" to see the logs connect to before the
> modularization.


Yes indeed,
most of the time you want to do "git log --follow".
There is a way to set "--follow" as a default option.


> Come to realize that this mayhem is part of the refactoring
> of the statistics library done in 2007(?).

Yes:
http://www.itk.org/Wiki/Proposals:Refactoring_Statistics_Framework_2007_Migration_Users_Guide


> Google a bit to find the wiki
> page that talks about the statistics refactoring of 2007 and a migration
> guide page that says that itkDecisionRuleBase was refactored into
> itkDecisionRule and removed the overloaded methods for Evaluate().
> So... what to do.
> Looks like last August, we (Hans) made the statistics library in the Review
> directory the default and removed the old statistics library(?). But this
> left behind the itkDecisionRuleBase and subclasses. I expect these were
> either left because (a) there were actually in Common not Statistics so it
> wasn't obvious that they needed to be removed, or (b) left for some level of
> backward compatibility.
> What do we want to do with these classes from the original statistics
> library?  We could:
> 1) Remove them. This has a corollary of what to do with the *2 versions of
> the classes. Should we rename them?


I would vote for this option.

This is our one-in-a-decade chance to do clean up.


> 2) Keep them as is.
> 3) Keep them but have them use the new classes.
>
>
> Jim Miller
> Senior Scientist
> GE Research
> Interventional and Therapy
> GE imagination at work
>
> _______________________________________________
> 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
>
>