lmacken / quantumrandom

Tools for utilizing the ANU Quantum Random Number Generator
https://pypi.python.org/pypi/quantumrandom
146 stars 36 forks source link

randint() now supports ranges greater than 65535 #12

Closed ekimekim closed 11 years ago

ekimekim commented 11 years ago

Previous behaviour caused program to hang. New upper limit on range is essentially arbitrary.

It does this by, instead of retrieving a uint16, it retrieves a number of uint16s and uses a bit shift to construct a larger-bit random number. It then performs the standard anti-modulo-bias algorithm on that.

lmacken commented 11 years ago

Thanks for the patch, Mike! I'll spin up a new release and push it to PyPi now.

lmacken commented 11 years ago

Hmm, I can't seem to get the randint unit test working with your patch. Can you give it a look when you get a chance?

ekimekim commented 11 years ago

Ok, so I found the problem and fixed it, but due to a series of unfortunate coincidences (mostly involving a crappy out-of-date ubuntu install that I swear i'll get around to dist-upgrading one day), I can't actually push to github at the moment. The tests now pass. I'll remember to check that next time. I can push this commit tomorrow, but if you want to fix this quickly, here's the git show of the commit:

Fix an off-by-one error when using range as a bit width.

This was causing a hang when range == 1,
and would have also resulted in a value of 65536 never being seen
in a request like randint(0,65536).

diff --git a/quantumrandom/__init__.py b/quantumrandom/__init__.py
index eab0f37..3f39663 100644
--- a/quantumrandom/__init__.py
+++ b/quantumrandom/__init__.py
@@ -85,7 +85,7 @@ def randint(min=0, max=10, generator=None):
     if generator is None:
         generator = cached_generator()

-    source_bits = int(math.ceil(math.log(range, 2)))
+    source_bits = int(math.ceil(math.log(range + 1, 2)))
     source_size = int(math.ceil(source_bits / float(INT_BITS)))
     source_max = 2**(source_size * INT_BITS) - 1
lmacken commented 11 years ago

Awesome, thanks for tracking this down! I'll let you commit it tomorrow so you get attribution for the fix. No rush, since I didn't push a new version out yet.

Instead of dist-upgrading Ubuntu, you should just install Fedora ;) (I'm extremely biased, since I work on Fedora for a living)

ekimekim commented 11 years ago

Actually I'm firmly in the ArchLinux camp :P, my laptop's just old and I don't use it often. Thanks.

lmacken commented 11 years ago

Cool :)

If you can't manage to push your fix to github, feel free to send me a git formatted patch instead.

Cheers.