-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial implementation of read counting in C read parser #877
Conversation
|
|
Also, any thoughts about |
retest this please |
static | ||
PyObject * | ||
ReadParser_get_num_reads(khmer_ReadParser_Object * me, PyObject * args) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, my thoughts exactly. A pox on all your .class
es.
Was unaware of tp_members
, my Python.h
knowledge is limited :)
Will investigate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented accessor-based access in 526fe19
92f59c4
to
348d7b0
Compare
Ready for review, modulo a possible name change 'num_reads' => 'read_count' |
348d7b0
to
5757f2f
Compare
5757f2f
to
1c5f8a2
Compare
Rebased on master, (actual code) conflicts resolved. |
@@ -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 ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should only be incremented if ret == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, fixed
LGTM, make it mergeable and I'll do so. |
Also accessor for python-land
f9e42fa
to
34764ae
Compare
That should be good to go @mr-c |
Initial implementation of read counting in C read parser
Thanks @kdmurray91 ! |
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