Merge pull request dib-lab#727 from ged-lab/doc/new-cpython-checklist

update ReadAligner to modern Python class + docs

ChangeLog

@@ -1,3 +1,9 @@
+2015-02-19  Michael R. Crusoe  <mcrusoe@msu.edu>
+
+   * doc/dev/coding-guidelines-and-review.txt: added checklist for new CPython
+   types
+   * khmer/_khmermodule.cc: Update ReadAligner to follow the new guidelines
+
 2015-02-19  Daniel Standage  <daniel.standage@gmail.com>
 
    * Makefile: add a new Makefile target `help` to list and describe all

doc/dev/coding-guidelines-and-review.txt

@@ -76,3 +76,36 @@ ready for review::
 **Note** that after you submit the comment you can check and uncheck
 the individual boxes on the formatted comment; no need to put x or y
 in the middle.
+
+CPython Checklist
+-----------------
+
+Here's a checklist for new CPython types with futureproofing for Python 3::
+
+   - [ ] the CPython object name is of the form `khmer_${OBJECTNAME}_Object`
+   - [ ] Named struct with `PyObject_HEAD` macro
+   - [ ] `static PyTypeObject khmer_${OBJECTNAME}_Type` with the following
+     entries
+      - [ ] `PyVarObject_HEAD_INIT(NULL, 0)` as the object init (this includes
+        the `ob_size` field).
+      - [ ] all fields should have their name in a comment for readability
+      - [ ] The `tp_name` filed is a dotted name with both the module name and
+        the name of the type within the module. Example: `khmer.ReadAligner`
+      - [ ] Deallocator defined and cast to `(destructor)` in tp_dealloc
+        - [ ] The object's deallocator must be
+          `Py_TYPE(obj)->tp_free((PyObject*)obj);`
+      - [ ] Do _not_ define a `tp_getattr`
+      - [ ] BONUS: write methods to present the state of the object via
+        `tp_str` & `tp_repr`
+      - [ ] _Do_ pass in the array of methods in `tp_methods`
+      - [ ] _Do_ define a new method in `tp_new`
+   - [ ] PyMethodDef arrays contain doc strings
+      - [ ] Methods are cast to `PyCFunctions`s
+   - [ ] Type methods use their type Object in the method signature.
+   - [ ] Type creation method decrements the reference to self
+     (`Py_DECREF(self);`) before each error-path exit (`return NULL;`)
+   - [ ] No factory methods. Example: `khmer_new_readaligner`
+   - [ ] Type object is passed to `PyType_Ready` and its return code is checked
+     in `init_khmer()`
+   - [ ] The reference count for the type object is incremented before adding
+     it to the module: `Py_INCREF(&khmer_${OBJECTNAME}_Type);`.

khmer/__init__.py

@@ -14,7 +14,7 @@
 from khmer._khmer import _LabelHash
 from khmer._khmer import _Hashbits
 from khmer._khmer import _HLLCounter
-from khmer._khmer import new_readaligner  # sandbox/{ec,error-correct-pass2}.py
+from khmer._khmer import ReadAligner
 
 from khmer._khmer import forward_hash  # figuregen/*.py
 # tests/test_{functions,counting_hash,labelhash,counting_single}.py

khmer/_khmermodule.cc

@@ -712,7 +712,7 @@ static PyTypeObject khmer_KSubsetPartitionType = {
 typedef struct {
     PyObject_HEAD
     ReadAligner * aligner;
-} khmer_ReadAlignerObject;
+} khmer_ReadAligner_Object;
 
 static void khmer_counting_dealloc(PyObject *);
 
@@ -1348,8 +1348,8 @@ static PyObject * hash_get_tags_and_positions(PyObject * self, PyObject * args)
     while (!kmers.done()) {
         HashIntoType kmer = kmers.next();
         if (set_contains(counting->all_tags, kmer)) {
-             posns.push_back(pos);
-             tags.push_back(kmer);
+            posns.push_back(pos);
+            tags.push_back(kmer);
         }
         pos++;
     }
@@ -4063,11 +4063,9 @@ static PyTypeObject khmer_KLabelHashType = {
     0,                       /* tp_alloc */
 };
 
-static PyObject * readaligner_align(PyObject * self, PyObject * args)
+static PyObject * readaligner_align(khmer_ReadAligner_Object * me,
+                                    PyObject * args)
 {
-    khmer_ReadAlignerObject * me = (khmer_ReadAlignerObject *) self;
-    ReadAligner * aligner = me->aligner;
-
     const char * read;
 
     if (!PyArg_ParseTuple(args, "s", &read)) {
@@ -4080,8 +4078,7 @@ static PyObject * readaligner_align(PyObject * self, PyObject * args)
         return NULL;
     }*/
 
-    Alignment * aln;
-    aln = aligner->Align(read);
+    Alignment * aln = me->aligner->Align(read);
 
     const char* alignment = aln->graph_alignment.c_str();
     const char* readAlignment = aln->read_alignment.c_str();
@@ -4093,36 +4090,57 @@ static PyObject * readaligner_align(PyObject * self, PyObject * args)
 }
 
 static PyMethodDef khmer_ReadAligner_methods[] = {
-    {"align", readaligner_align, METH_VARARGS, ""},
-    {NULL, NULL, 0, NULL}
+    {"align", (PyCFunction)readaligner_align, METH_VARARGS, ""},
+    {NULL} /* Sentinel */
 };
 
-static PyObject *
-khmer_readaligner_getattr(PyObject * obj, char * name)
-{
-    return Py_FindMethod(khmer_ReadAligner_methods, obj, name);
-}
-
 //
 // khmer_readaligner_dealloc -- clean up readaligner object
 // GRAPHALIGN addition
 //
-static void khmer_readaligner_dealloc(PyObject* self)
+static void khmer_readaligner_dealloc(khmer_ReadAligner_Object* obj)
 {
-    khmer_ReadAlignerObject * obj = (khmer_ReadAlignerObject *) self;
     delete obj->aligner;
     obj->aligner = NULL;
+    Py_TYPE(obj)->tp_free((PyObject*)obj);
 }
 
+//
+// new_readaligner
+//
+static PyObject* khmer_ReadAligner_new(PyTypeObject *type, PyObject * args,
+                                       PyObject *kwds)
+{
+    khmer_ReadAligner_Object * self;
+
+    self = (khmer_ReadAligner_Object *)type->tp_alloc(type, 0);
+
+    if (self != NULL) {
+        khmer_KCountingHashObject * ch = NULL;
+        unsigned short int trusted_cov_cutoff = 2;
+        double bits_theta = 1;
+
+        if(!PyArg_ParseTuple(args, "O!Hd", &khmer_KCountingHashType, &ch,
+                             &trusted_cov_cutoff, &bits_theta)) {
+            Py_DECREF(self);
+            return NULL;
+        }
+
+        self->aligner = new ReadAligner(ch->counting, trusted_cov_cutoff,
+                                        bits_theta);
+    }
+
+    return (PyObject *) self;
+}
 
 static PyTypeObject khmer_ReadAlignerType = {
-    PyObject_HEAD_INIT(NULL)
-    0,
-    "ReadAligner", sizeof(khmer_ReadAlignerObject),
-    0,
-    khmer_readaligner_dealloc,     /*tp_dealloc*/
+    PyVarObject_HEAD_INIT(NULL, 0) /* init & ob_size */
+    "khmer.ReadAligner",		    /*tp_name*/
+    sizeof(khmer_ReadAligner_Object),	    /*tp_basicsize*/
+    0,					    /*tp_itemsize*/
+    (destructor)khmer_readaligner_dealloc,  /*tp_dealloc*/
     0,                          /*tp_print*/
-    khmer_readaligner_getattr,     /*tp_getattr*/
+    0,			        /*tp_getattr*/
     0,                          /*tp_setattr*/
     0,                          /*tp_compare*/
     0,                          /*tp_repr*/
@@ -4137,36 +4155,26 @@ static PyTypeObject khmer_ReadAlignerType = {
     0,                          /*tp_as_buffer*/
     Py_TPFLAGS_DEFAULT,         /*tp_flags*/
     "ReadAligner object",           /* tp_doc */
+    0,		               /* tp_traverse */
+    0,		               /* tp_clear */
+    0,		               /* tp_richcompare */
+    0,		               /* tp_weaklistoffset */
+    0,		               /* tp_iter */
+    0,		               /* tp_iternext */
+    khmer_ReadAligner_methods,  /* tp_methods */
+    0,             /* tp_members */
+    0,                         /* tp_getset */
+    0,                         /* tp_base */
+    0,                         /* tp_dict */
+    0,                         /* tp_descr_get */
+    0,                         /* tp_descr_set */
+    0,                         /* tp_dictoffset */
+    0,			       /* tp_init */
+    0,                         /* tp_alloc */
+    khmer_ReadAligner_new,     /* tp_new */
 };
 
 
-//
-// new_readaligner
-//
-static PyObject* new_readaligner(PyObject * self, PyObject * args)
-{
-    khmer_KCountingHashObject * ch = NULL;
-    unsigned short int trusted_cov_cutoff = 2;
-    double bits_theta = 1;
-
-    if(!PyArg_ParseTuple(args, "O!Hd", &khmer_KCountingHashType, &ch,
-                         &trusted_cov_cutoff, &bits_theta)) {
-        return NULL;
-    }
-
-    khmer_ReadAlignerObject * readaligner_obj = (khmer_ReadAlignerObject *) \
-            PyObject_New(khmer_ReadAlignerObject, &khmer_ReadAlignerType);
-
-    if (readaligner_obj == NULL) {
-        return NULL;
-    }
-
-    readaligner_obj->aligner = new ReadAligner(ch->counting, trusted_cov_cutoff,
-            bits_theta);
-
-    return (PyObject *) readaligner_obj;
-}
-
 //
 // new_hashbits
 //
@@ -4632,10 +4640,6 @@ static PyMethodDef KhmerMethods[] = {
         "_new_hashbits",        _new_hashbits,
         METH_VARARGS,       "Create an empty hashbits table"
     },
-    {
-        "new_readaligner",        new_readaligner,
-        METH_VARARGS,             "Create a read aligner object"
-    },
     {
         "forward_hash",     forward_hash,
         METH_VARARGS,       "",
@@ -4698,6 +4702,9 @@ init_khmer(void)
     if (PyType_Ready(&khmer_KHLLCounter_Type) < 0) {
         return;
     }
+    if (PyType_Ready(&khmer_ReadAlignerType) < 0) {
+        return;
+    }
 
     PyObject * m;
     m = Py_InitModule3( "_khmer", KhmerMethods,
@@ -4728,6 +4735,8 @@ init_khmer(void)
 
     Py_INCREF(&khmer_KHLLCounter_Type);
     PyModule_AddObject(m, "_HLLCounter", (PyObject *)&khmer_KHLLCounter_Type);
+    Py_INCREF(&khmer_ReadAlignerType);
+    PyModule_AddObject(m, "ReadAligner", (PyObject *)&khmer_ReadAlignerType);
 }
 
 // vim: set ft=cpp sts=4 sw=4 tw=79:

sandbox/collect-variants.py

@@ -65,7 +65,7 @@ def main():
         print 'making hashtable'
         ht = khmer.new_counting_hash(K, HT_SIZE, N_HT)
 
-    aligner = khmer.new_readaligner(ht, args.trusted_cutoff, args.bits_theta)
+    aligner = khmer.ReadAligner(ht, args.trusted_cutoff, args.bits_theta)
 
     if args.details_out is not None:
         details_out = open(args.details_out, "w")

sandbox/correct-errors.py

@@ -93,7 +93,7 @@ def main():
     print 'making hashtable'
     ht = khmer.new_counting_hash(K, HT_SIZE, N_HT)
 
-    aligner = khmer.new_readaligner(ht, args.trusted_cov, args.bits_theta)
+    aligner = khmer.ReadAligner(ht, args.trusted_cov, args.bits_theta)
 
     tempdir = tempfile.mkdtemp('khmer', 'tmp', args.tempdir)
     print 'created temporary directory %s; use -T to change location' % tempdir

sandbox/ec.py

@@ -26,7 +26,7 @@ def main():
     outfp = open(output_filename, 'w')
 
     ht = khmer.load_counting_hash(hash_filename)
-    aligner = khmer.new_readaligner(ht, 1, C, max_error_region)
+    aligner = khmer.ReadAligner(ht, 1, C, max_error_region)
 
     K = ht.ksize()
 

sandbox/error-correct-pass2.py

@@ -55,7 +55,7 @@ def main():
     # the filtering function.
     def process_fn(record):
         # read_aligner is probably not threadsafe?
-        aligner = khmer.new_readaligner(ht, 1, C, max_error_region)
+        aligner = khmer.ReadAligner(ht, 1, C, max_error_region)
 
         name = record['name']
         seq = record['sequence']

sandbox/normalize-by-align.py

@@ -59,7 +59,7 @@ def main():
         print 'making hashtable'
         ht = khmer.new_counting_hash(K, HT_SIZE, N_HT)
 
-    aligner = khmer.new_readaligner(ht, args.trusted_cutoff, args.bits_theta)
+    aligner = khmer.ReadAligner(ht, args.trusted_cutoff, args.bits_theta)
 
     if args.details_out != None:
         details_out = open(args.details_out, "w")

sandbox/read_aligner.py

@@ -41,7 +41,7 @@ def main():
     ht = khmer.load_counting_hash(counting_ht)
     K = ht.ksize()
 
-    aligner = khmer.new_readaligner(ht, args.trusted_cov, args.theta) # counting hash, trusted kmer coverage cutoff, bits theta (threshold value for terminating unproductive alignemnts)
+    aligner = khmer.ReadAligner(ht, args.trusted_cov, args.theta) # counting hash, trusted kmer coverage cutoff, bits theta (threshold value for terminating unproductive alignemnts)
 
     ### the filtering loop
     for infile in infiles:

tests/test_read_aligner.py

@@ -15,7 +15,7 @@ def eq_(v1, v2):
 def test_alignnocov():
     ch = khmer.new_counting_hash(10, 1048576, 1)
     read = "ACCTAGGTTCGACATGTACC"
-    aligner = khmer.new_readaligner(ch, 0, 0)
+    aligner = khmer.ReadAligner(ch, 0, 0)
     for i in range(20):
         ch.consume("AGAGGGAAAGCTAGGTTCGACAAGTCCTTGACAGAT")
     ch.consume("ACCTAGGTTCGACATGTACC")
@@ -28,7 +28,7 @@ def test_alignnocov():
 
 def test_simple_readalign():
     ch = khmer.new_counting_hash(10, 1048576, 1)
-    aligner = khmer.new_readaligner(ch, 2, 0)
+    aligner = khmer.ReadAligner(ch, 2, 0)
     for i in range(20):
         ch.consume("AGAGGGAAAGCTAGGTTCGACATGTCCTTGACAGAT")
     read = "ACCTAGGTTCGACAAGTACC"
@@ -47,7 +47,7 @@ def test_simple_readalign():
 
 def test_readalign():
     ch = khmer.new_counting_hash(10, 1048576, 1)
-    aligner = khmer.new_readaligner(ch, 1, 0)
+    aligner = khmer.ReadAligner(ch, 1, 0)
     for i in range(20):
         ch.consume("AGAGGGAAAGCTAGGTTCGACAAGTCCTTGACAGAT")
     read = "ACCTAGGTTCGACATGTACC"
@@ -208,7 +208,7 @@ def test_readalign():
 
 def test_readalign_new():
     ch = khmer.new_counting_hash(32, 1048576, 1)
-    aligner = khmer.new_readaligner(ch, 1, 0)
+    aligner = khmer.ReadAligner(ch, 1, 0)
     for seq in ht_seqs:
         ch.consume(seq)