Skip to content

Conversation

@standage
Copy link
Member

@standage standage commented Aug 8, 2017

Each variant of the hashtable consume_seqfile method is overloaded to support either an input filename (provided as a std::string) or alternatively a read parser. However, most of these are not currently accessible via the Python API. This PR adds these to the CPython code.

While running the tests, I ran into "too many open files" errors. On further investigation, none of the existing CPython code explicitly closed the read parsers. I addressed this for both the new methods as well as the existing method.

One point of concern: the proliferation of consume functions is getting out of hand. We can only postpone refactoring for so long.

Ready for review and merge!

  • Is it mergeable?
  • make test Did it pass the tests?
  • make clean diff-cover If it introduces new functionality in
    scripts/ is it tested?
  • make format diff_pylint_report cppcheck doc pydocstyle Is it well
    formatted?



PyObject *
hashtable_consume_seqfile_with_reads_parser(khmer_KHashtable_Object * me,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this function up for consistency's sake: for each overloaded method, the version that accepts a string is first, and the version that accepts a parser is second.


PyObject *
hashtable_consume_seqfile_with_reads_parser(khmer_KHashtable_Object * me,
PyObject * args)
Copy link
Member Author

@standage standage Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like git is struggling with the similarity between all of the variants of this function and the fact that I cut n pasted this function further up, again for consistency. I promise the CPython code changes are a lot more boring than this diff would suggest. :-)

@codecov-io
Copy link

codecov-io commented Aug 8, 2017

Codecov Report

Merging #1753 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1753      +/-   ##
=========================================
- Coverage    0.05%   0.05%   -0.01%     
=========================================
  Files          87      90       +3     
  Lines       11451   11561     +110     
  Branches     3078    3118      +40     
=========================================
  Hits            6       6              
- Misses      11445   11555     +110
Impacted Files Coverage Δ
src/khmer/_cpy_hashtable.cc 0% <0%> (ø) ⬆️
khmer/__init__.py 2.66% <0%> (-0.34%) ⬇️
src/khmer/_cpy_khmer.cc 0% <0%> (ø) ⬆️
src/oxli/hashgraph.cc 0% <0%> (ø) ⬆️
khmer/_oxli/oxli_exception_convert.cc 0% <0%> (ø) ⬆️
src/oxli/hllcounter.cc 0% <0%> (ø) ⬆️
src/oxli/read_parsers.cc 0% <0%> (ø) ⬆️
src/khmer/_cpy_graphlabels.cc 0% <0%> (ø) ⬆️
src/oxli/labelhash.cc 0% <0%> (ø) ⬆️
src/khmer/_cpy_hashgraph.cc 0% <0%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41e175b...83ad21c. Read the comment docs.

@standage
Copy link
Member Author

Hey @camillescott and @ctb: I'm not grokking the errors from the latest CI build for this thread. Do you think this has to do with CPython inheritance issues?

@standage
Copy link
Member Author

With most recent Cython PRs merged, closing in favor of #1785.

@standage standage closed this Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants