-
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
HLL coverage improvements #738
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really tempted to use the buffer interface, but I think it is better to do it when implementing #714
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 |
df9afea
to
4dfca3a
Compare
|
fed6496
to
2e16872
Compare
5fcd005
to
2db412b
Compare
Ready for review @mr-c @ctb @camillescott |
NULL | ||
}, | ||
{NULL} /* Sentinel */ | ||
}; |
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.
khmer/_khmermodule.cc:4611:1: warning: deprecated conversion from string constant to ‘char*’ [-Wwrite-strings]
?
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.
So, I don't really get where is it coming from. Is it from the sentinel?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 [email protected] 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.