-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add 64bit Min Sketch #53
Conversation
This looks good, thanks! Why did the CI fail? |
@piskvorky sorry my bad I didn't check that the CI runs on python 3.4 and 3.5, I've added f-strings that's probably why these are failing. As for 3.7 I have no clue because 3.6 seems ok I'll check it now. |
The project CI runs on 3.4 and 3.5 which do not support f-strings.
@piskvorky I've fixed the string's formatting issue and it works on all versions except 3.7, looking at the logs it seems the error is comming from setuptools rather than the bounter package as it fails at the Maybe trying a newer version of 3.7 such as 3.7.9 instead of 3.7.1 fixes the CI pipeline.
|
I've tried it on my computer using python 3.7.1 installed via I've also found that this error has already been reported to pypa pypa/setuptools#3293, maybe updating setuptools via the requirements files fixes the issue in the 3.7.1 image used by travis. |
@mpenkov could you please check? This looks like a useful change, let's get it merged in and released. Many thanks. |
Hi @mpenkov thanks for fixing the issue with Python 3.7. Sadly now it failed in 3.6, but it is weird, as before all tests passed and all now all other versions are passing them too. My guess is that there is a rounding error that may or may not happen as the offending test is self.assertAlmostEqual(self.cms['big number'], big_number, delta=self.delta * big_number) Can you rerun https://app.travis-ci.com/github/RaRe-Technologies/bounter/jobs/587411973? If the error is due to floating point rounding maybe it was just bad luck that we had that rounding error. |
Yeah, seems like a flickering test. Everything passes now. I'll leave you some minor comments, then we should be good to merge. |
Merged! Thank you for your work @jponf |
Added a new CMS with cell size uint64_t. This should address #51.
To control the cell size I've added a new parameter so the user has a choice to use 32 or 64 bit. By default is set to 32 bit so old code should work exactly the same.
I'm open to address any issue or concern that you may have.