-
Notifications
You must be signed in to change notification settings - Fork 295
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
Streaming unique-kmers.py using consume_fasta #1204
Conversation
797124f
to
e13f5f9
Compare
cc #1053 |
e13f5f9
to
1d003d2
Compare
Update: I moved |
|
|
||
if (!PyArg_ParseTuple(args, "s", &filename)) { | ||
if (!PyArg_ParseTuple(args, "s|O", &filename, &output_records_o)) { |
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.
Should output_records
be a keyword on this method? I think it makes more sense, because calling it is a bit weird (check https://github.com/dib-lab/khmer/pull/1204/files#diff-7f09af630bd7b293c0827bba1d411617R120 for an example).
This also raises the question: do we want to do this in other places? We do have a lot of default arguments in the extension glue code that are not exposed nicely to Python land...
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.
keyword is fine by me; how about a post-2.0 issue for the 2nd question?
1d003d2
to
f7b94d0
Compare
a4716b4
to
01abd68
Compare
84106a2
to
28abbda
Compare
LGTM. Merge once the tests pass. |
Streaming unique-kmers.py using consume_fasta
Revert
unique-kmers.py
changes and use consume_fasta again. I wrote awrite_record
equivalent in C++ for this, currently living inlib/HLLCounter.cc
, but I think this should probably go inlib/read_parsers.cc
.(reopening #1177 now that #1176 was merged)