search

graehl / pygr

Automatically exported from code.google.com/p/pygr
0 stars 0 forks source link

Code review: seqdb.py #66

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
Due: Feb 24th

Original issue reported on code.google.com by the.good...@gmail.com on 11 Feb 2009 at 1:51

GoogleCodeExporter commented 9 years ago 
See: 
http://groups.google.com/group/pygr-dev/browse_thread/thread/4a2d1648101a4b78
and

http://groups.google.com/group/pygr-dev/browse_thread/thread/f03601c835c6bc57

Original comment by the.good...@gmail.com on 18 Mar 2009 at 7:20

GoogleCodeExporter commented 9 years ago 
We completed most of this in the merging of the seqdb-review branch.  Titus, are
there PrefixUnionDict issues that you want to discuss as part of this review?

Original comment by cjlee...@gmail.com on 8 Apr 2009 at 1:55

GoogleCodeExporter commented 9 years ago 
See

http://groups.google.com/group/pygr-dev/browse_thread/thread/f03601c835c6bc57/14
25a52a7f67c33d?lnk=gst&q=prefixuniondict#1425a52a7f67c33d

where Chris says:

"""
I'll answer the PrefixUnionDict questions in a separate email. 
"""

There are a few unanswered questions in there still, I think: to quote,

SeqPrefixUnionDict is oddly named.

PrefixUnionDict could contain __iadd__ behavior (instead of
SeqPrefixUnionDict, which inherits from it solely to add __iadd__
behavior).  Why not do things this way?

There's an 'if' statement in SeqPrefixUnionDict.__iadd__ that can
never be reached, as far as I can tell.  Could someone (Chris) please
verify that for me?

In two places (SeqPrefixUnionDict.__iadd__, find @CTB untested; and
_PrefixUnionDictInverse.__getitem__, find @CTB abstraction) explicit
knowledge of annotation objects is assumed in classes that otherwise
don't know anything special about them.  Any way to deal with this
better or should I just suck up and deal & write some tests?

A few PrefixUnionDict issues:

 - PrefixUnionDict.__contains__ can take either a sequence ID or a
   sequence object.  This is inconsistent with __getitem__ (which only
   takes seq IDs).  Ugly.

 - PrefixUnionDict.__init__ takes an argument 'trypath' that is really a
   list of paths to try.  Could we rename this to 'trypaths'?  Or should
   I just suck up and deal & write a nice doc?

 - PrefixUnionDict.writeHeaderFile function (etc.) isn't used anywhere
   in the code.  How useful is it?

 - PrefixUnionDict.newMemberDict(...) and the associated
   _PrefixUnionDictMember class don't have a clear purpose to me.
   Could someone give me a concrete use case?  If not, maybe they should
   be removed or something; they're confusing!

 - _PrefixUnionDictMember.default handling could probably be written
   in a more standard way using dict.setdefault, no?  How much code depends
   on this specific behavior?

Original comment by the.good...@gmail.com on 8 Apr 2009 at 2:01

GoogleCodeExporter commented 9 years ago

Original comment by the.good...@gmail.com on 26 Apr 2009 at 8:47

GoogleCodeExporter commented 9 years ago 
All changes have been reviewed and all the code has been committed to master. 
Closing the issue.

Original comment by mare...@gmail.com on 13 May 2009 at 2:05

GoogleCodeExporter commented 9 years ago 
See comment 3.  Am I missing something?

Original comment by the.good...@gmail.com on 13 May 2009 at 2:24

GoogleCodeExporter commented 9 years ago

Original comment by mare...@gmail.com on 10 Jun 2009 at 1:47