[Insight-developers] Insight code walkthrough
Miller, James V (CRD)
millerjv@crd.ge.com
Tue, 18 Sep 2001 17:11:39 -0400
This message is in MIME format. Since your mail reader does not understand
this format, some or all of this message may not be legible.
------_=_NextPart_000_01C14086.821C69A0
Content-Type: multipart/alternative;
boundary="----_=_NextPart_001_01C14086.821C69A0"
------_=_NextPart_001_01C14086.821C69A0
Content-Type: text/plain;
charset="iso-8859-1"
Bill, Will and I performed a code walk-through on Insight yesterday. We looked through the code in
BasicFilters and (part of) Algorithms. We only got through about 100 objects. We looked at each
object for:
Inheritance: Did class use the correct superclass
Documentation: Was the description of the class/methods appropriate
Set/Get: Did the class use the SetMacros and GetMacros to properly manage modified
times
Progress: Did the class properly manage the progress value.
PrintSelf: Did the class provide and properly implement a PrintSelf method.
Name: Was the name of the class appropriate
Group: Was the class in the correct Doxygen group (using \ingroup tag)
Threaded: Was the class a multithreaded filter?
Streams: Did the class properly manage the pipeline mechanism so it would support
streaming
Notes: Observations on design
Directory: Was the class in the correct directory
DebugMacros: Did the class use DebugMacros or std::cout
The attached Excel spreadsheet details our analysis. This is checked into the repository under the
Insight/Documentation directory. Actually, Kitware refuses to send out the spreadsheet through their
mailer since it is "too big" (100KB). So people will have to do an update on the cvs repository.
Not all of categories are pertinent to all classes. If the category was not appropriate for a class,
the designation of "NA" was used. For instance, classes that are not filters have an NA entry for
"Threaded" and "Streams". Also, filters that do not implement a GenerateData() or a
ThreadedGenerateData() but rather use an implementation of these methods from their superclass will
also have NA for "Threaded" and "Streams".
Fields in the spreadsheet marked red are items that need to be fixed. (Class names marked red are
slated to be removed from the system in favor of other designs). If these items are not addressed,
then certain filters will not work properly in an application that relies on the pipeline mechanism
updating properly. These definately need to be fixed by the Beta release. We ask that when you fix a
class listed in the speadsheet, you notify Bill, Will or myself. We will re-evaluate your class and
modify the spreadsheet appropriately.
We made a cursory pass at determining whether a given filter should stream and thread. There are
algorithms that cannot be threaded or streamed. However, there were many classes that could stream
and be threaded that were not implemented that way. They should be fixed. If you feel that we
incorrectly labelled your class, then let us know.
General comments:
* Overall, the system isn't too bad off. Every developer has their own patterns of mistakes or
ommissions. For instance, I am usually negligent on supporting Progress and PrintSelf. Other
developers avoid using the Set/Get macros. Other are not providing UpdateOutputInformation(),
EnlargeOutputRequestedRegion(), GenerateInputRequestedRegion(), or GenerateOutputRequestedRegion()
where they are needed.
* We need to find a way to reduce the amount of whitespace in the header files. Perhaps we can
condense documentation before each method to a single line comment block or group typedefs so that
they do not have a separate comment block (especially for typedefs that inherited from the
superclass).
* It looks like we need a few other doxygen groups. I'd like one for the morphology classes.
We need another one that will encompass classes like Pad, Flip, Join, Extract (ImageManipulation ?)
* We'd like to have doxygen groups for "Threaded" and "Streams" to indicate whether a filter is
multithreaded and whether it will stream without requiring all the input and produce all the output.
* We need a doxygen group for exceptions
* We will add to the SetVectorMacro so that a second method will be define automatically that
takes a single scalar and sets every element in the vector to that value.
* We need to add a PrintSelf test since there are many many PrintSelf defects. Ivars that a
user can set or get need to be printed in the PrintSelf method.
* We need to add a test to see a filter supports progress methods.
* In many places we use C-style casts for convert between pixel types. Should we use
static_cast<> instead.
* We need to install LaTeX on public again. Doxygen cannot format its equations without it.
* GenerateData() frequently is listed as a public method. It should always be protected.
* UpateOutputInformation() etc is typically public. We will look into whether it can be
protected since ProcessObject and DataObject are friends.
* PrintSelf() may be moved into the public section. Users should never call PrintSelf(). They
should call Print() or use operator<< to a stream. Classes, however, could call PrintSelf() on
another object but this should only be done within their own PrintSelf() method.
We'll be getting back together to continue assessing the Algorithms directory and Common and
Numerics.
Jim Miller
_____________________________________
Visualization & Computer Vision
GE Corporate Research & Development
Bldg. KW, Room C218B
P.O. Box 8, Schenectady NY 12301
millerjv@crd.ge.com < mailto:millerjv@crd.ge.com <mailto:millerjv@crd.ge.com> >
(518) 387-4005, Dial Comm: 8*833-4005,
Cell: (518) 505-7065, Fax: (518) 387-6981
Jim Miller
_____________________________________
Visualization & Computer Vision
GE Corporate Research & Development
Bldg. KW, Room C218B
P.O. Box 8, Schenectady NY 12301
millerjv@crd.ge.com < mailto:millerjv@crd.ge.com <mailto:millerjv@crd.ge.com> >
(518) 387-4005, Dial Comm: 8*833-4005,
Cell: (518) 505-7065, Fax: (518) 387-6981
------_=_NextPart_001_01C14086.821C69A0
Content-Type: text/html;
charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META HTTP-EQUIV=3D"Content-Type" CONTENT=3D"text/html; =
charset=3Diso-8859-1">
<META content=3D"MSHTML 5.50.4611.1300" name=3DGENERATOR></HEAD>
<BODY>
<DIV><FONT size=3D2>
<DIV><SPAN class=3D199134213-18092001>Bill, Will and I performed a code =
walk-through on Insight yesterday. We looked through the code in=20
BasicFilters and (part of) Algorithms. We only got through about =
100=20
objects. We looked at each object for:</SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Inheritance: &nb=
sp; =20
Did class use the correct superclass</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Documentation: &=
nbsp; Was=20
the description of the class/methods appropriate</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Set/Get: &=
nbsp; =20
Did the class use the SetMacros and GetMacros to properly manage =
modified=20
times</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Progress: =
=20
Did the class properly manage the progress value.</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>PrintSelf:  =
; =20
Did the class provide and properly implement a PrintSelf =
method.</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Name: &nbs=
p; &nbs=
p; =20
Was the name of the class appropriate</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Group: &nb=
sp; &nb=
sp; =20
Was the class in the correct Doxygen group (using \ingroup =
tag)</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Threaded: =
=20
Was the class a multithreaded filter?</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Streams: &=
nbsp; =20
Did the class properly manage the pipeline mechanism so it would =
support=20
streaming</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Notes: &nb=
sp; &nb=
sp; =20
Observations on design</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>Directory:  =
; =20
Was the class in the correct directory</SPAN></DIV>
<DIV><SPAN=20
class=3D199134213-18092001>DebugMacros: &nb=
sp; =20
Did the class use DebugMacros or std::cout</SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001>The attached Excel spreadsheet =
details our=20
analysis. This is checked into the repository under the=20
Insight/Documentation directory.<SPAN class=3D525550921-18092001> =
<STRONG><FONT=20
color=3D#ff0000>Actually, Kitware refuses to send out the spreadsheet =
through=20
their mailer since it is "too big" (100KB). So people will have to do =
an update=20
on the cvs repository.</FONT></STRONG></SPAN></SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001>Not all of categories are =
pertinent to all=20
classes. If the category was not appropriate for a class, the =
designation=20
of "NA" was used. For instance, classes that are not filters have =
an NA=20
entry for "Threaded" and "Streams". Also, filters that do not =
implement a=20
GenerateData() or a ThreadedGenerateData() but rather use an =
implementation of=20
these methods from their superclass will also have NA for =
"Threaded" and=20
"Streams".</SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001>Fields in the spreadsheet marked =
red are=20
items that need to be fixed. (Class names marked red are slated to be =
removed=20
from the system in favor of other designs). If these items are not =
addressed,=20
then certain filters will not work properly in an application that =
relies on the=20
pipeline mechanism updating properly. <STRONG>These definately =
need to be=20
fixed by the Beta release.</STRONG> We ask that when you fix a class =
listed in=20
the speadsheet, you notify Bill, Will or myself. We will =
re-evaluate your=20
class and modify the spreadsheet appropriately. </SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001>We made a cursory pass at =
determining=20
whether a given filter should stream and thread. There are =
algorithms that=20
cannot be threaded or streamed. However, there were many classes =
that=20
could stream and be threaded that were not implemented that way. =
They=20
should be fixed. If you feel that we incorrectly labelled your class, =
then let=20
us know. </SPAN><SPAN class=3D199134213-18092001></SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001>General comments:</SPAN></DIV>
<UL>
<LI><SPAN class=3D199134213-18092001>Overall, the system isn't too =
bad=20
off. Every developer has their own patterns of mistakes or=20
ommissions. For instance, I am usually negligent on supporting =
Progress=20
and PrintSelf. Other developers avoid using the Set/Get macros. =
Other=20
are not providing UpdateOutputInformation(), =
EnlargeOutputRequestedRegion(),=20
=
GenerateInputRequestedRegion(), or GenerateOutputRequestedRegi=
on()=20
where they are needed.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>We need to find a way to reduce =
the amount=20
of whitespace in the header files. Perhaps we can condense =
documentation=20
before each method to a single line comment block or group typedefs =
so that=20
they do not have a separate comment block (especially for typedefs =
that=20
inherited from the superclass). </SPAN>=20
<LI><SPAN class=3D199134213-18092001>It looks like we need a few =
other doxygen=20
groups. I'd like one for the morphology classes. We need =
another=20
one that will encompass classes like Pad, Flip, Join, Extract=20
(ImageManipulation ?) </SPAN>=20
<LI><SPAN class=3D199134213-18092001>We'd like to have doxygen groups =
for=20
"Threaded" and "Streams" to indicate whether a filter is =
multithreaded and=20
whether it will stream without requiring all the input and produce =
all the=20
output.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>We need a doxygen group for=20
exceptions</SPAN>=20
<LI><SPAN class=3D199134213-18092001>We will add to the =
SetVectorMacro so that a=20
second method will be define automatically that takes a single scalar =
and sets=20
every element in the vector to that value.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>We need to add a PrintSelf test =
since there=20
are many many PrintSelf defects. Ivars that a user can set or =
get need=20
to be printed in the PrintSelf method.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>We need to add a test to see a =
filter=20
supports progress methods.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>In many places we use C-style =
casts for=20
convert between pixel types. Should we use static_cast<>=20
instead.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>We need to install LaTeX on =
public=20
again. Doxygen cannot format its equations without it.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>GenerateData() frequently is =
listed as a=20
public method. It should always be protected.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>UpateOutputInformation() etc is =
typically=20
public. We will look into whether it can be protected since=20
ProcessObject and DataObject are friends.</SPAN>=20
<LI><SPAN class=3D199134213-18092001>PrintSelf() may be moved into =
the public=20
section. Users should never call PrintSelf(). They should =
call=20
Print() or use operator<< to a stream. Classes, however, =
could=20
call PrintSelf() on another object but this should only be done =
within their=20
own PrintSelf() method.</LI></UL></SPAN>
<DIV><SPAN class=3D199134213-18092001>We'll be getting back together to =
continue=20
assessing the Algorithms directory and Common and =
Numerics.</SPAN></DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><SPAN class=3D199134213-18092001></SPAN> </DIV>
<DIV><B><FONT face=3D"Comic Sans MS" color=3D#000080>Jim =
Miller</FONT></B>=20
<BR><B><I><FONT face=3DArial=20
color=3D#ff0000>_____________________________________</FONT></I></B><I><=
/I><BR><I></I><I><FONT=20
face=3DArial color=3D#000000 size=3D1>Visualization & Computer =
Vision<BR>GE=20
Corporate Research & Development<BR>Bldg. KW, Room C218B<BR>P.O. =
Box 8,=20
Schenectady NY 12301<BR><BR></FONT><U><FONT face=3DArial =
color=3D#0000ff=20
size=3D1>millerjv@crd.ge.com <<A=20
href=3D"mailto:millerjv@crd.ge.com">mailto:millerjv@crd.ge.com</A>></=
FONT></U></I><BR><I><FONT=20
face=3DArial color=3D#000000 size=3D1>(518) 387-4005, Dial Comm: =
8*833-4005,=20
</FONT></I><BR><I><FONT face=3DArial color=3D#000000 size=3D1>Cell: =
(518) 505-7065,=20
Fax: (518) 387-6981</FONT></I> </DIV><BR>
<DIV> </DIV></FONT></DIV><BR>
<P><B><FONT face=3D"Comic Sans MS" color=3D#000080>Jim =
Miller</FONT></B>=20
<BR><B><I><FONT face=3DArial color=3D#ff0000=20
size=3D2>_____________________________________</FONT></I></B><I></I><BR>=
<I></I><I><FONT=20
face=3DArial color=3D#000000 size=3D1>Visualization & Computer =
Vision<BR>GE=20
Corporate Research & Development<BR>Bldg. KW, Room C218B<BR>P.O. =
Box 8,=20
Schenectady NY 12301<BR><BR></FONT><U><FONT face=3DArial =
color=3D#0000ff=20
size=3D1>millerjv@crd.ge.com <<A=20
href=3D"mailto:millerjv@crd.ge.com">mailto:millerjv@crd.ge.com</A>></=
FONT></U></I><BR><I><FONT=20
face=3DArial color=3D#000000 size=3D1>(518) 387-4005, Dial Comm: =
8*833-4005,=20
</FONT></I><BR><I><FONT face=3DArial color=3D#000000 size=3D1>Cell: =
(518) 505-7065,=20
Fax: (518) 387-6981</FONT></I> </P><BR>
<DIV> </DIV></BODY></HTML>
------_=_NextPart_001_01C14086.821C69A0--
------_=_NextPart_000_01C14086.821C69A0
Content-Type: application/octet-stream;
name="Miller, James V (CRD).vcf"
Content-Disposition: attachment;
filename="Miller, James V (CRD).vcf"
BEGIN:VCARD
VERSION:2.1
N:Miller;James
FN:Miller, James V (CRD)
ORG:CRD;ESL
TITLE:Computer Scientist
TEL;WORK;VOICE:*833-4005
TEL;WORK;VOICE:1 518 387-4005
ADR;WORK:;KW-C218B;P.O. Box 8;Schenectady;New York;12301;USA
LABEL;WORK;ENCODING=QUOTED-PRINTABLE:KW-C218B=0D=0AP.O. Box 8=0D=0ASchenectady, New York 12301=0D=0AUSA
EMAIL;PREF;INTERNET:millerjv@crd.ge.com
REV:20010420T140329Z
END:VCARD
------_=_NextPart_000_01C14086.821C69A0--