Initial implementation of read counting in C read parser #877

kdm9 · 2015-03-19T04:49:52Z

Hi all,

I've been thinking about #676, and decided to jump right in with one pre-requisite of metadata reporting, counting reads.

This is an implementation of read counting for the C parser(s), and an accessor for python-land objects.

It appears threadsafe, from my hacky testing (increments are done within spinlocked code blocks). I'll test further, and write tests etc as time permits :)

Cheers,
K

kdm9 · 2015-03-19T04:50:21Z

Is it mergeable?
Did it pass the tests?
If it introduces new functionality in scripts/ is it tested?
Check for code coverage with make clean diff-cover
Is it well formatted? Look at make pep8, make diff_pylint_report,
make cppcheck, and make doc output. Use make format and manual
fixing as needed.
Did it change the command-line interface? Only additions are allowed
without a major version increment. Changing file formats also requires a
major version number increment.
Is it documented in the ChangeLog?
http://en.wikipedia.org/wiki/Changelog#Format
Was a spellchecker run on the source code and documentation after
changes were made?

kdm9 · 2015-03-19T04:50:23Z

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 PyCFunctionss
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);.

kdm9 · 2015-03-19T04:52:41Z

ping @mr-c, since #676 is your issue :)

kdm9 · 2015-03-19T04:54:30Z

Also, any thoughts about _num_reads vs _read_count as the method's name?

kdm9 · 2015-03-19T05:15:58Z

retest this please

mr-c · 2015-03-20T09:12:30Z

khmer/_khmermodule.cc

+static
+PyObject *
+ReadParser_get_num_reads(khmer_ReadParser_Object * me, PyObject * args)
+{


readparser.get_num_reads() is a bit Java-y. You can implement this value as a simple attribute (accessed via readparser.num_reads via tp_members (https://docs.python.org/2/extending/newtypes.html#generic-attribute-management)

Yup, my thoughts exactly. A pox on all your .classes.

Was unaware of tp_members, my Python.h knowledge is limited :)

Will investigate this.

Hang on, how about using tp_getsetters, a la the HLLCounter's alpha and error_rate? From what I can see, it would be difficult to expose a protected member of the IParser class via the khmer_ReadParser_Object struct.

However, would love to stand corrected

Implemented accessor-based access in 526fe19

kdm9 · 2015-03-20T12:05:03Z

Ready for review, modulo a possible name change 'num_reads' => 'read_count'

kdm9 · 2015-03-28T12:10:43Z

Rebased on master, (actual code) conflicts resolved.

mr-c · 2015-03-28T15:53:04Z

lib/read_parsers.cc

@@ -57,6 58,7 @@ void SeqAnParser::imprint_next_read(Read &the_read)
 if (!atEnd) {
 ret = seqan::readRecord(the_read.name, the_read.sequence,
 the_read.quality, _private->stream);
+ _num_reads ;


this should only be incremented if ret == 0

Indeed, fixed

mr-c · 2015-03-30T14:02:17Z

LGTM, make it mergeable and I'll do so.

Also accessor for python-land

kdm9 · 2015-03-30T21:40:56Z

That should be good to go @mr-c

Initial implementation of read counting in C read parser

mr-c · 2015-03-31T07:39:51Z

Thanks @kdmurray91 !

mr-c reviewed Mar 20, 2015
View reviewed changes

kdm9 added a commit that referenced this pull request Mar 20, 2015

Add ReadParser.num_reads #877 to changelog

92f59c4

kdm9 force-pushed the feat/rparser-nreads branch from 92f59c4 to 348d7b0 Compare March 20, 2015 11:56

kdm9 added a commit that referenced this pull request Mar 20, 2015

Add ReadParser.num_reads #877 to changelog

348d7b0

kdm9 force-pushed the feat/rparser-nreads branch from 348d7b0 to 5757f2f Compare March 25, 2015 01:56

kdm9 added a commit that referenced this pull request Mar 25, 2015

Add ReadParser.num_reads #877 to changelog

5757f2f

kdm9 force-pushed the feat/rparser-nreads branch from 5757f2f to 1c5f8a2 Compare March 28, 2015 12:03

kdm9 added a commit that referenced this pull request Mar 28, 2015

Add ReadParser.num_reads #877 to changelog

1c5f8a2

mr-c reviewed Mar 28, 2015
View reviewed changes

kdm9 mentioned this pull request Mar 29, 2015

Behaviour of SeqAnParser with a truncated fastq #884

Closed

kdm9 added 8 commits March 31, 2015 08:38

Initial implementation of read counting in IParser

455268b

Also accessor for python-land

Add tests of ReadParser.get_num_reads()

bec2592

Slay the many headed beast that is 'make pep8'

f209de3

Pylint-ify test_read_parsers.py

470db18

Use cpython object accessors instead of methods

870c49b

Add ReadParser.num_reads #877 to changelog

44b05d7

Increment n_reads IFF read is parsed successfully

9c67349

Remove extra initialisation in SeqAnParser ctor

34764ae

kdm9 force-pushed the feat/rparser-nreads branch from f9e42fa to 34764ae Compare March 30, 2015 21:40

mr-c added a commit that referenced this pull request Mar 31, 2015

Merge pull request #877 from ged-lab/feat/rparser-nreads

0bfaf8e

Initial implementation of read counting in C read parser

mr-c merged commit 0bfaf8e into master Mar 31, 2015

mr-c deleted the feat/rparser-nreads branch March 31, 2015 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of read counting in C read parser #877

Initial implementation of read counting in C read parser #877

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

mr-c Mar 20, 2015

kdm9 Mar 20, 2015

kdm9 Mar 20, 2015

kdm9 Mar 20, 2015

kdm9 commented Mar 20, 2015

kdm9 commented Mar 28, 2015

mr-c Mar 28, 2015

kdm9 Mar 29, 2015

mr-c commented Mar 30, 2015

kdm9 commented Mar 30, 2015

mr-c commented Mar 31, 2015

Initial implementation of read counting in C read parser #877

Initial implementation of read counting in C read parser #877

Conversation

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

kdm9 commented Mar 19, 2015

mr-c Mar 20, 2015

Choose a reason for hiding this comment

kdm9 Mar 20, 2015

Choose a reason for hiding this comment

kdm9 Mar 20, 2015

Choose a reason for hiding this comment

kdm9 Mar 20, 2015

Choose a reason for hiding this comment

kdm9 commented Mar 20, 2015

kdm9 commented Mar 28, 2015

mr-c Mar 28, 2015

Choose a reason for hiding this comment

kdm9 Mar 29, 2015

Choose a reason for hiding this comment

mr-c commented Mar 30, 2015

kdm9 commented Mar 30, 2015

mr-c commented Mar 31, 2015