HLL coverage improvements #738
Conversation
luizirber
force-pushed
the
tests/hll_coverage
branch
8 times, most recently
from
ffdb830 to
330c194
Compare
|
Ready for review @mr-c @ctb @camillescott |
|
|
||
| PyObject * x = PyList_New(counters.size()); | ||
| for (size_t i = 0; i < counters.size(); i++) { | ||
| PyList_SET_ITEM(x, i, PyLong_FromLong(counters[i])); |
There was a problem hiding this comment.
I was really tempted to use the buffer interface, but I think it is better to do it when implementing #714
luizirber
force-pushed
the
tests/hll_coverage
branch
from
330c194 to
14ba417
Compare
|
@luizirber As soon as this is made mergable and #738 (comment) is address I think it will be ready to go |
luizirber
force-pushed
the
tests/hll_coverage
branch
3 times, most recently
from
df9afea to
4dfca3a
Compare
|
luizirber
force-pushed
the
tests/hll_coverage
branch
2 times, most recently
from
fed6496 to
2e16872
Compare
luizirber
force-pushed
the
tests/hll_coverage
branch
2 times, most recently
from
5fcd005 to
2db412b
Compare
|
Ready for review @mr-c @ctb @camillescott |
| NULL | ||
| }, | ||
| {NULL} /* Sentinel */ | ||
| }; |
There was a problem hiding this comment.
khmer/_khmermodule.cc:4611:1: warning: deprecated conversion from string constant to ‘char*’ [-Wwrite-strings] ?
There was a problem hiding this comment.
So, I don't really get where is it coming from. Is it from the sentinel?
There was a problem hiding this comment.
I get multiple warnings for the same line number so I'm guessing it is all the conversions within the code block.
There was a problem hiding this comment.
I get multiple warnings for the same line number so I'm guessing it is all
the conversions within the code block.
On Tue, Feb 24, 2015, 19:06 Luiz Irber notifications@github.com wrote:
In khmer/_khmermodule.cc
#738 (comment):
- {
(char const *)"ksize",(getter)hllcounter_get_ksize, (setter)hllcounter_set_ksize,(char const *)"k-mer size for this HLL counter.""Can be changed prior to first counting, but becomes read-only after ""that (raising AttributeError)",NULL- },
- {
(char const *)"counters",(getter)hllcounter_getcounters, NULL,(char const *)"Read-only internal counters.",NULL- },
- {NULL} /* Sentinel */
+};So, I don't really get where is it coming from. Is it from the sentinel?
—
Reply to this email directly or view it on GitHub
https://github.com/ged-lab/khmer/pull/738/files#r25306902.
There was a problem hiding this comment.
Found it, shouldn't have used const.
|
Can you eliminate that warning and cover those last two bits with tests? Otherwise, it LGTM; I love the output of |
luizirber
force-pushed
the
tests/hll_coverage
branch
from
760b071 to
c4bfbf0
Compare
|
@mr-c fixed remaining issues |
HLL coverage improvements
HyperLogLog line coverage is a bit low because some conditions during estimation are not so easy to trigger. After exposing the HLL internals it was easier to find test cases using data already available for the current tests.
This PR improves the coverage and also simplifies some error checking inside the HLL.