[Insight-developers] Operators with explicitly specified coef ficients

[Miller, James V (Research)]

Gunnar,

You can submit code fixes to the list and a developer will move them into
the code base.  The best approach, however, is to enter a bug in the
bug-tracker and attach the fix there.  This way it will not be lost.  Anyone
can enter a bug (you may need to create an account on the bug-tracker).  The
bug tracker can be found at 

http://www.itk.org/Bug/index.php

We do have a document on the coding conventions. But I think the conventions
constructors are limited to "large" objects. For large objects, we use named
constructors (T::New()) and make the default constructor private and we do
not
use copy constructors or constructors that take arguments.

For "smaller" objects, typically stacked based objects, the rules are
looser.
So the only rule is to be consistent with similar objects in the system.

The coding style document can be found in Insight/Documentation/Style.pdf

Jim



-----Original Message-----
From: Gunnar Farneback [mailto:gunnar at bwh.harvard.edu]
Sent: Thursday, July 08, 2004 5:19 PM
To: millerjv at crd.ge.com; insight-developers at public.kitware.com
Subject: Re: [Insight-developers] Operators with explicitly specified
coefficients


> But I think it makes to have an operator that does this.  Perhaps one
> that is not limited to directional kernels as well.

I wouldn't mind a more general one as well, but I'm not sure it really
makes sense to have them in the same class. In any case I'm not enough
up to speed with C++ and ITK to design that.

> As for ITK style issues, here are a few minor points:
> 
> 1. The ITK style limits abbreviations to just a few abbreviations in 
> widespread acceptance.  So the methods Set/GetCoeffs() should be written
> out as SetCoefficients()/GetCoefficients().

Fixed.

> 2. You probably need a default constructor that will initialize the 
> direction ivar and fill the coefficient vector with good default values
> (maybe 1 at the center pixel and zeros elsewhere).

I chose 0 at the center pixel and no coefficients elsewhere
(technically speaking). This way it should be obvious if one forgets
to set own values, getting all zero output rather than a copy of the
input. I hope this isn't inconsistent with any ITK philosophy.

> 3. DirectionalOperator(int direction) and DirectionalOperator(int
direction,
> const std::vector<double> coeffs) - I am not sure about having these two 
> additional constructors.  I don't think the other operators have
> constructors
> that take "direction" as an argument.  So to be consistent, this operator
> should probably only have the default constructor and the copy
constructor.
> Users can use the SetDirection() and SetCoefficients() methods to populate
> the operator.

Actually I'd prefer to only allow setting direction and coefficients
directly from a constructor, but of course consistency is more
important. I've removed these.

Is there any design document which discusses for example use of
constructors?

I also removed the internal calls to CreateDirectional(), to be more
consistent with other operators.

> 4. ITK style is to preface calls to member functions with "this->", so
> instead
> of 
> 	SetDirection(direction);
> 
> we use
> 
> 	this->SetDirection(direction);

Fixed.

> 5. You have a duplication of "data".  The coefficient vector is stored in 
> an ivar and then again in the neighborhood.  You might want to look at 
> trying to eliminate your ivar to hold the coefficient vector and just have
> the coefficients stored in the neighborhood.

I've looked but failed to find any attractive way to eliminate it. I'm
open for suggestions if it's obvious to somebody else how to do it.

Take two of itkDirectionalOperator.h is attached. Is the name of the
class appropriate or should it be named something else?

I also have some minor and obvious bugfixes (mostly documentation) for
GaussianOperator and NeighborhoodOperator. Should I send patches to
this list or directly to some developer, if so who?

/Gunnar