Add fastLog2 operation, closes #67 #71
Conversation
src/bigints.nim
Outdated
| @@ -959,3 +959,5 @@ iterator `..<`*(a, b: BigInt): BigInt = | |||
| while res < b: | |||
| yield res | |||
| inc res | |||
|
|
|||
| func fastLog2*(a: BigInt): int = bitops.fastLog2(a.limbs[a.limbs.high]) + 32*(a.limbs.len-1) | |||
There was a problem hiding this comment.
Minor: Put the body of the proc in a separate line, and add doc comment.
|
Ok, maybe the implementation is fast, but should that be reflected in the name of the function? Why not just |
@narimiran I do not want to say that my implementation is fast, but I want the user to understand this is the equivalent for BigInt of the |
|
Hmm, I wonder what would be best for negative numbers. I think calculating log2 of the absolute value makes sense, but then it should at least be documented. Alternatively, we could throw an exception, but I don't know how useful that would be. |
Integrate changes of toSignedInt PR
Specified edge case Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
src/bigints.nim
Outdated
| ## If `a` is zero, returns -1. | ||
| if a.isZero: | ||
| return -1 | ||
| bitops.fastLog2(a.limbs[a.limbs.high]) + 32*(a.limbs.len-1) |
There was a problem hiding this comment.
a.limbs.high and a.limbs.len-1 are the same thing. Why use both, and not just one of those variants?
| @@ -385,6 +385,20 @@ proc main() = | |||
| doAssert "fedcba9876543210".initBigInt(base = 16) == b | |||
| doAssert "ftn5qj1r58cgg".initBigInt(base = 32) == b | |||
|
|
|||
| block: # fastLog2 | |||
| let a = one shl 31 | |||
There was a problem hiding this comment.
We need some "not-power-of-two" tests too.
There was a problem hiding this comment.
Do you have any idea of easily checkable tests then ?
I have tested the fastLog of a+b which is not a power of 2.
I have added one test but it is not obvious what is it’s logarithm.
There are not much possible errors with this simple function and I am tired of waiting 30 seconds every time I have to test my code. It is much longer now with all the GC’s.
Maybe some factorial, since with Stirling approximation, we can guess the number of digits ahead ?
There was a problem hiding this comment.
I'd also test values with all 1's, e.g. one shl 64 n- 1 and a few negative non-powers-of-two.
As for running the tests yourself, you can just nim r tests/tbigints.nim. (In theory, the behavior should be the same for all GCs, but in practice, there are bugs, as exemplified by #88.)
There was a problem hiding this comment.
Do you have any idea of easily checkable tests then ?
I would do some sanity checks, e.g. log of 7, 8, 9, 24, 32, 48, etc., just to make sure it works for the basics. And then do what @konsumlamm suggests.
|
Are the new tests sufficient ? |
Have you read #71 (comment) and #71 (comment)? |
|
I have, I added tests corresponding to these comments and I am asking if the new tests are okay for you. |
|
Huh, for some reason your latest commit doesn't shop up here... Maybe you can try to rebase/merge the master branch. |
|
I was some commits behind master. That might be the cause. |
Add a bitops-like operation for BigInts with respective test.
Solves #67.