Re: [OpenBabel-Devel] API-review; ideas for better interface-documentation. Was: forcefield classes

[Tim V. <tim...@gm...>]

On 2/18/07, Carsten Niehaus <cni...@gm...> wrote:
>
> Am Sonntag, 18. Februar 2007 05:00 schrieb Tim Vandermeersch:
> > The ghemical  forcefield is now completely implemented.
>
> > All the energy expressions and graphs on the wiki for the ghemical
> > forceifeld are also finished.
>
> Tim, I read a bit of your code today. A lot API-docs are missing. Look for
> example here and you'll notice a lot gaps:


 Yes, the link below has a lot of documentation missing (note: the link
below is updated monthly, so it isn't up-to-date with svn). I'm currently
working on this and it allready looks alot better.

http://openbabel.sourceforge.net/dev-api/classOpenBabel_1_1OBForceField.shtml
>
> I hope it is ok that I comment a bit about this here: For example, in
>
>   svn/openbabel/src/forcefields/forcefieldghemical.h
>
> you have a lot code similar to this:
>
>   class OBFFVDWCalculationGhemical : public OBFFCalculation
>   {
>     public:
>       OBAtom *a, *b; // atoms of the pair
>       double ka, Ra, kb, Rb, kab, rab, sigma, sigma6, sigma12;
>
>       double GetEnergy();
>       vector3 GetGradient(OBAtom *atom);
>   };
>
> Four comments:
> 1. If doxygen works the way I think it works this class (actually, the 6
> classes derived from OBFFCalculation in that file) will not be picked up
> by
> doxygen because the class itself is not wrapped in doxygen.


This is true, but at the moment nothing in the src/forcefields/ directory is
included by doxygen. The 6 derived classes do the same as the base class,
but have additional variables to hold results. These results are only needed
by the associated OBForceFieldYYYY class.
Do we need API documentation on the files in src/forcefields/ (there is no
documentation for src/formats/ files)?
A solution to this could be doxygen pages with information about each
forcefield and a tutorial/guide/template to adding your own forcefield.
End users (developers of projects using OB) would probably only be
interested in the OBForceField class. All other classes are only for
internal use.

2. I doubt that the OBAtom's and the double's need to be public. private
> should be ok, right?


You need to be able to access them. When you calculate the bond stretching
energy in E_Bond() you iterate over the vector of OBFFBondCalculationYYYY's
and add the return values of GetEnergy(). When you want to output
information about the individual interactions (atom types, rab, ...),
E_Bond() needs to access the doubles and OBAtoms in the
OBFFBondCalculationYYYY. Functions like
OBFFBondCalculationYYYY::GetForceConstant(),
OBFFBondCalculationYYYY::GetIdealBondLength() could be used when the
variables are private.

3. Both methods (Get*) are not documented. As stated in the first comment I
> think they will not be correctly displayed in the APIdocs... please add
> a "@return ..." comment, if possible (or tell me what they do and I will
> send
> you a patch)


The Get* methods for the base calss are now documented.

4. I am not sure if the name OBFFVDWCalculationGhemical is a good one. After
> reading the code I _think_ you moved the code from Ghemical to OB or
> something like that but the code is not really related to Ghemical. In
> other
> words: The name shouldn't contain the string "Ghemical" because there is
> no
> relation.


The reason why the name contains Ghemical is because this is the VDW
implementation (energy expression) for the ghemical forcefield (=relation).
A similar OBFFVDWCalculationMMFF94 and OBFFVDWCalculationMM2 exists. General
energy expressions could also be added (to forcefield.cpp) e.g.
OBFFVDWCalculationBuckingHam, OBFFVDWCalculationLennardJones, ... these
could be used by specific forcefields instead of difining their own.

The MM wiki page contains some information about the naming of methods (with
the XXX's and YYYY's). Let me know if this is understandable or just
confusing.

5. I found this page in the wike:
>
>   http://openbabel.sourceforge.net/wiki/Molecular_mechanics
>
> I really think that these information belong into the code (of course as
> doxygen-comments!) so that I would have found them ealier and they are
> easier
> to be keept uptodate. Doxygen can be a really powerful tool. I think that
> specific page should be move into a "Mainpage.dox"-file which would result
> in
> a page similar to


I agree. I noticed that you can include images in the doxygen comments.
Would it be a good idea to include the graphs from the wiki page? What would
be the best way to get the equations in the doxygen documentation (render to
image, does doxygen support tex)?

  http://api.kde.org/cvs-api/kdelibs-apidocs/phonon/html/index.html
> or
>   http://api.kde.org/cvs-api/kdelibs-apidocs/khtml/html/index.html
>
> Look especially at the Phonon-page. It is the perfect example of really
> really
> good API-documentation which looks good and is interlinked a lot.
> You can see how such a Mainpage.dox is done for example here:
>
>
> http://websvn.kde.org/trunk/KDE/kdelibs/phonon/Mainpage.dox?rev=630516&view=markup
>
> I am of course willing to help you! I think that API-docs are _really_
> important. I also think this because I intend to use OB more and more in
> the
> future and that I am currently a bit lost in the API ;-)


As  I'm new to doxygen help is always welcome.

Later tonight I will commit a new version of forcefield.h with updated
doxygen comments. The documentation is also split up using:
//! \name ...
//@{
...
//@}
(current names: Methods for energy evaluation, Methods for logging, Methods
for structure generation,  Methods for  energy minimization, Methods for
validation)

Thanks for the patch.

Tim