[BioRuby] Codeml parser

[Naohisa GOTO]

Hi Pjotr,

On Tue, 5 Jan 2010 11:32:12 +0100
Pjotr Prins <pjotr.public14 at thebird.nl> wrote:

> Hi Naohisa,
> 
> First I thought you were kidding. But then I realise you are serious.
> 
> I don't think we need to document every simple class variable/accessor
> to accept this source code. That is overkill. If you don't understand
> lnL or alpha, don't use it. We are not in the business of documenting
> for documenting's sake.  Documenting lnL and alpha will be like:
> 
> "Retrieve the lnL value from the Report" 
> 
> "Retrieve the alpha value from the Report" 
> 
> etc. etc. I don't think we should be doing that. Standard 1-to-1
> relations are obvious and don't need lots of text in the code base.

Even just one word is OK, e.g. "lnL", "alpha".
But no RDoc is not allowed.

Ideally, it may be really great if well informative description
can help people unfamiliar with Codeml, and this may encourage
people beginning to use Codeml with BioRuby. I understand this
can not be easily achieved. When writing a new class or largely
adding codes, it is also good to implement first with least
documentation and later to improve documents gradually.

> If someone feels like filling in these obvious statements, fine. It
> really goes against my grain. Do we document every single accessor?
> Note the previous implementation did no such thing. That code was
> accepted fine (and partially written by you).

In late 2005, we determined that all methods, attributes, classes,
modules, etc. should be documented by using RDoc. Codes written
before earlier 2006 may have no RDoc. I'm working to add RDoc in
such codes gradually, but not finished yet.

> > Details of +buf+ (class, contents, etc) should also be written in RDoc.
> > It is recommended to use the style written in the README_DEV.rdoc, or
> > the style used in the Ruby source code.
> 
> You mean the contents of the input buffer, which is the content of the
> input file? I see many places in Bioruby where no such a thing is
> done.  Why become strict on this now? If you want a different
> descriptive name for the variable - that is fine. Propose me
> a better name.

No need to change the variable name. I mean I want to clarify
that it points contents of the file and not filename.
If you think current description is enough apparent, it is OK.

> > >      def to_a
> > >        [ @position, @aaref, @probability, @omega ]
> > >      end
> > What is the purpose of the method?
> 
> Access converter. Convenience, really. You can remove it if you
> dislike it so much. I use it for testing and to write to a file. Could
> be to_s too, but that fixates the format.

OK if you feel useful.

> > >    class PositiveSites < Array
> > 
> > To inherit Array and to create original container class is discouraged.
> > In BioRuby, we have deprecated Bio::Features and Bio::References in
> > version 1.3.0, although they do not inherit Array but have an array
> > in the object. (The classes still exist only for backward compatibility,
> > in lib/bio/compat/features.rb and references.rb).
> 
> PositiveSites object has the all the features of a list (ie Array). I
> think inheritance is what it should be. It is an is_a relationship.
> Adding a @list will just add code. Not only for initialization, but
> also for iterators. I only see how we can move backwards from readable
> code. Nor is it good OOP practice. Inheritance is not *always* bad,
> though I agree it is used too quickly (in general).
> 
> > In this case, except initialize, only a method named "graph" is added.
> > I think it is good to add the graph method in the Report class and
> > using an Array for storing PositiveSite objects.
> 
> This is awful. The graph is a feature of PositiveSites, and not of the
> report *parser*. To keep things simple it is best practise to have
> functionality where it belongs. It is good OOP design. Your proposal
> means the Report class becomes less obvious in what it is. Look how
> clean it is now!

I respect your design if the class is not only a container of
PositiveSite objects but also having methods doing special things
by using relations among two or more objects which is not a simple
accumulation of each object's information.

> What do other people think on this list. I am at a disadvantage here.
>
> I would like this code accepted in Bioruby, so other people can use
> it. I disagree with most of above 'criticism'. I certainly balk at the
> last non-OOP ones. This is not the first time I am really unhappy. I
> can't believe how much trouble I have to go to for a simple class,
> which, as it happens, has a perfectly acceptable implementation by
> most measures.
> 
> Pj.
> 

Naohisa Goto
ngoto at gen-info.osaka-u.ac.jp / ng at bioruby.org